Last modified: 2013-04-22 16:17:07 UTC
Created attachment 8264 [details] proposed patch When there is a piece with lots of opening braces at the stack, which should get three or more elements, just the two topmost are recognised. The rest will be handled as $skippedBraces even when it would be enough to create an element. This problem of course is very rare and and can be simply worked around by some whitspaces, but I guess this behavior is not intended. I tried to write a patch, hoping not to create PHP syntax errors :-)
adding some keywords
Comment on attachment 8264 [details] proposed patch >Index: Preprocessor_DOM.php >=================================================================== >--- Preprocessor_DOM.php (Revision 83528) >+++ Preprocessor_DOM.php (Arbeitskopie) >@@ -611,18 +611,12 @@ > $piece->count -= $matchingCount; > # do we still qualify for any callback with remaining count? >- $names = $rules[$piece->open]['names']; >- $skippedBraces = 0; >- $enclosingAccum =& $accum; >- while ( $piece->count ) { >- if ( array_key_exists( $piece->count, $names ) ) { >- $stack->push( $piece ); >- $accum =& $stack->getAccum(); >- break; >- } >- --$piece->count; >- $skippedBraces ++; >- } >- $enclosingAccum .= str_repeat( $piece->open, $skippedBraces ); >+ $min = $rules[$piece->open]['min']; >+ if ( $piece->count >= $names->min ) { >+ $stack->push( $piece ); >+ $accum =& $stack->getAccum(); >+ } else { >+ $accum .= str_repeat( $piece->open, $piece->count ); >+ } > } > $flags = $stack->getFlags(); > extract( $flags ); >Index: Preprocessor_Hash.php >=================================================================== >--- Preprocessor_Hash.php (Revision 83528) >+++ Preprocessor_Hash.php (Arbeitskopie) >@@ -624,18 +624,12 @@ > $piece->count -= $matchingCount; > # do we still qualify for any callback with remaining count? >- $names = $rules[$piece->open]['names']; >- $skippedBraces = 0; >- $enclosingAccum =& $accum; >- while ( $piece->count ) { >- if ( array_key_exists( $piece->count, $names ) ) { >- $stack->push( $piece ); >- $accum =& $stack->getAccum(); >- break; >- } >- --$piece->count; >- $skippedBraces ++; >- } >- $enclosingAccum->addLiteral( str_repeat( $piece->open, $skippedBraces ) ); >+ $min = $rules[$piece->open]['min']; >+ if ( $piece->count >= $min ) { >+ $stack->push( $piece ); >+ $accum =& $stack->getAccum(); >+ } else { >+ $accum .= str_repeat( $piece->open, $piece->count ); >+ } > } > > extract( $stack->getFlags() );
Created attachment 8273 [details] testcase
Created attachment 8274 [details] expected testresult
Created attachment 8275 [details] proposed patch
Sorry, I didn't want to comment the patch, I tried to edit it. Forget comment 2 :-) preprocessor-prasertests uploaded.
Could you maybe write a parserTest (phase3/tests/parser/parserTests.txt); I can't seem to understand the testcase (what is the input, what is result, what is the expected result)
I'm sorry, I don't understand the syntax of these parser tests, and http://www.mediawiki.org/wiki/Parser_tests is still a redlink. It should be a preprocessor test, like those in phase3/tests/parser/preprocess/. The problem is, when there are lots of opening braces, their closing does not work for more than two stack elements. You can see this at "{{{{{{ }} }} }}", which renders to "{{<tpl><tpl> </tpl> </tpl> }}". My test is just like a cartesian product of possibilities of open and close pairs/triples, I may should shorten it.
Created attachment 8859 [details] Single unified patch against phase3 with code changes and (passing!) parser test I just took the patch and the proposed test, fixed a few whitespace issues with the test, applied the patch, set up the test, and the test is passing. This patch incorporates the three initial attachments.
Dan, thank you for the patch and the parser test! They are very much appreciated. I'm notifying Gabriel Wicke, who is working on rewriting the parser. In the few months since you made attachment 8859 [details], the trunk has changed a little; would you mind updating it to work against MediaWiki as it is in Subversion now? We'd appreciate that. Thank you. I'm sorry for the delay and the inconvenience.
Comment on attachment 8859 [details] Single unified patch against phase3 with code changes and (passing!) parser test Per automated testing http://lists.wikimedia.org/pipermail/wikitech-l/2011-November/056340.html patch no longer applies to MediaWiki trunk in Subversion. Requesting update.
Created attachment 9549 [details] Updated patch against r104190 What's this, Sumana, you're working on thanksgiving? Per manual testing, the only failing hunk from the original patch had the effect of removing a single line, which by the way was already commented out. Here is the updated patch.
Dan, I am basically taking the long weekend off, but I find that gardening Bugzilla is a little bit calming, like how some people pop the bubbles in bubble wrap. :-) Added the "patch" and "need-review" keywords to signal that there's a patch here that awaits review. Please do feel free to add them the next time you attach a new patch that another developer should review. Thanks for the update.
See Gerrit change #8511
Merged -> marking as fixed.
