Last modified: 2014-09-08 11:47:46 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T30642, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 28642 - wrong part->commentEnd set to atributting comments
wrong part->commentEnd set to atributting comments
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
1.17.x
All All
: Normal normal (vote)
: ---
Assigned To: Bergi
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-21 13:13 UTC by Bergi
Modified: 2014-09-08 11:47 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
proposed patch, I guess the $wsEnd is the problem (842 bytes, patch)
2011-04-21 13:16 UTC, Bergi
Details
correction (1.46 KB, patch)
2011-04-22 13:45 UTC, Bergi
Details
preprocessor test file (1.24 KB, text/plain)
2011-04-22 15:13 UTC, Bergi
Details
preprocessor test expection (3.85 KB, text/plain)
2011-04-22 15:14 UTC, Bergi
Details
applying the test files (6.46 KB, patch)
2011-04-23 12:58 UTC, Bergi
Details

Description Bergi 2011-04-21 13:13:56 UTC
To be able to search for headline-closing equal sign even when there are comments between the last = and the linebreak, there are some position values appended to the recent domParts. This works well with the first comment, but as soon as a second one comes, the commentEnd value is set to $wsEnd instead of $endPos.
This breaks the intended behavior when the second comment is followed by some blanks.
See the difference between http://en.wikipedia.org/wiki/Special:ExpandTemplates?generate_xml=1&input=%3D%3D%20X%20%3D%3D%20%3C!--%20--%3E%20%3C!--%20--%3E%20 and http://en.wikipedia.org/wiki/Special:ExpandTemplates?generate_xml=1&input=%3D%3D%20X%20%3D%3D%20%3C!--%20--%3E%20%3C!--%20--%3E (mind the trailing whitespace).
I guess it also should work at 
http://en.wikipedia.org/wiki/Special:ExpandTemplates?generate_xml=1&input=%3D%3D%20X%20%3D%3D%20%3C!--%20--%3E%20%3C!--%20--%3E%20%3C!--%20--%3E%20%3C!--%20--%3E%20 , but it doesn't.
Comment 1 Bergi 2011-04-21 13:16:19 UTC
Created attachment 8437 [details]
proposed patch, I guess the $wsEnd is the problem
Comment 2 Mark A. Hershberger 2011-04-21 22:22:01 UTC
Patch applied: r86676
Comment 3 Bergi 2011-04-22 13:45:20 UTC
Created attachment 8443 [details]
correction

Thanks for the fast handling, but as also Nikerabbit complained in the revision comments, it was untested.
And not only that, it even was wrong. Setting a value before testing it makes not much sense :-(

I also included the same patch for the preprocessor-hash.
Comment 4 Mark A. Hershberger 2011-04-22 14:20:20 UTC
Could you provide a unit test using FauxRequest and an API call to ExpandTemplates?  Even some sample code that I could mangle into a unit test would work.
Comment 5 Bergi 2011-04-22 15:00:28 UTC
The regexp for a heading would be
=+.+?=+[ \t]*(<!--[\s\S]*?--> *)*[ \t]*
with (fake)linebreaks on both sides. So I can give only some code examples.
I'm going to write a preprocessor test (/phase3/tests/parser/preprocess/), you may use its code for a unit test (or try /phase3/tests/phpunit/includes/parser/PreprocessorTest.php). Unfortunately I have no PHP running anywhere.
Comment 6 Mark A. Hershberger 2011-04-22 15:08:03 UTC
I've applied your updated patch in r86709.  If you provide an attempt at test, I'll make sure it works.
Comment 7 Bergi 2011-04-22 15:13:32 UTC
Created attachment 8446 [details]
preprocessor test file
Comment 8 Bergi 2011-04-22 15:14:23 UTC
Created attachment 8447 [details]
preprocessor test expection
Comment 9 Bergi 2011-04-22 19:36:08 UTC
Test works at translatewiki.
Comment 10 Platonides 2011-04-22 21:09:02 UTC
Can you provide the test as a patch to tests/phpunit/includes/parser/PreprocessorTest.php ?
(and fixing anything it may have broke)
Comment 11 Bergi 2011-04-23 12:58:15 UTC
Created attachment 8450 [details]
applying the test files

Theres nothing to change in provideCases(), and nothing is broken. The former broken things, like "=== Foo ===<!-- --> <!-- --> \n", were not included there, and "=== Foo ===<!-- --> <!-- -->\n" did already work.
You may extend the test files with these cases like "==<!-- -->= Foo === ", and you may extend the provideCases array with some tabulator cases.
Comment 12 Mark A. Hershberger 2011-04-23 21:27:53 UTC
Thanks! r86795

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links