Last modified: 2006-12-14 21:43:31 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 T9503, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 7503 - poem doesn't work with nested nowiki
poem doesn't work with nested nowiki
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
General/Unknown (Other open bugs)
unspecified
PC Linux
: Normal normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-05 22:44 UTC by Steve Sanbeg
Modified: 2006-12-14 21:43 UTC (History)
0 users

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


Attachments
fix for 1.8 (1.90 KB, patch)
2006-10-05 22:45 UTC, Steve Sanbeg
Details
requested test cases (552 bytes, text/plain)
2006-10-06 16:14 UTC, Steve Sanbeg
Details
fix problems with previous versions (2.18 KB, patch)
2006-10-06 17:14 UTC, Steve Sanbeg
Details
add regression case for previous patch (724 bytes, text/plain)
2006-10-06 17:15 UTC, Steve Sanbeg
Details
minor format changes, add one more test case (973 bytes, text/plain)
2006-10-06 19:17 UTC, Steve Sanbeg
Details
supress sputious error in previous version (2.23 KB, patch)
2006-10-09 16:21 UTC, Steve Sanbeg
Details
new patch, no static vars. (2.04 KB, patch)
2006-10-13 21:47 UTC, Steve Sanbeg
Details
added credits entry, so it will display better in special;version (4.00 KB, patch)
2006-10-23 16:20 UTC, Steve Sanbeg
Details
regression tests (1.23 KB, text/plain)
2006-10-23 18:44 UTC, Steve Sanbeg
Details
new patch, with fix for paragraph with leading space. (3.95 KB, patch)
2006-10-23 18:45 UTC, Steve Sanbeg
Details
Reimplemented as an extension (1.89 KB, text/plain)
2006-11-03 20:27 UTC, Steve Sanbeg
Details
regression tests for the extension (2.62 KB, text/plain)
2006-11-03 20:30 UTC, Steve Sanbeg
Details
updated patch to work with Rob's changes, etc. (3.95 KB, patch)
2006-11-22 19:57 UTC, Steve Sanbeg
Details
corrected patch. (1.62 KB, patch)
2006-11-22 19:59 UTC, Steve Sanbeg
Details
update regression tests to work with Simetrical's changes (1.37 KB, text/plain)
2006-11-22 19:59 UTC, Steve Sanbeg
Details

Description Steve Sanbeg 2006-10-05 22:44:27 UTC
text in <poem><nowiki> gets a <br> rendered as text at the end of each line;
using new hooks in 1.8 can prevent that.
Comment 1 Steve Sanbeg 2006-10-05 22:45:01 UTC
Created attachment 2461 [details]
fix for 1.8
Comment 2 Brion Vibber 2006-10-05 22:52:14 UTC
Can you provide some test cases to confirm the fix and detect regressions?

See extensions/Cite/citeParserTests.txt for an example;
run them with maintenance/parserTest.php --file=etc
Comment 3 Steve Sanbeg 2006-10-06 16:14:28 UTC
Created attachment 2462 [details]
requested test cases

1 - simple regerssion test for <poem>, for regression -w- current version
2 - example of <nowiki>, for context
3 - nested <poem><nowiki>, to exercise new feature
Comment 4 Steve Sanbeg 2006-10-06 17:14:12 UTC
Created attachment 2464 [details]
fix problems with previous versions

enhancement to fix problem with recursive text in pre 1.8; preven possible
bloating of strip state.
Comment 5 Steve Sanbeg 2006-10-06 17:15:05 UTC
Created attachment 2465 [details]
add regression case for previous patch
Comment 6 Steve Sanbeg 2006-10-06 19:17:18 UTC
Created attachment 2466 [details]
minor format changes, add one more test case

This should be the last change for awhile...
Comment 7 Brion Vibber 2006-10-09 01:07:58 UTC
This code generates PHP notices when run, indicating some sloppy checks:

Notice: Undefined index:  general in /opt/web/pages/trunk/extensions/Poem/Poem.php on line 37

Always test code with error_reporting set to E_ALL.


I'm also a bit uneasy about these static variable; it looks like after the
first <poem> tag, all others will behave differently because $tag is now set.
Comment 8 Steve Sanbeg 2006-10-09 16:21:24 UTC
Created attachment 2491 [details]
supress sputious error in previous version
Comment 9 Steve Sanbeg 2006-10-09 16:32:06 UTC
(In reply to comment #7)
> This code generates PHP notices when run, indicating some sloppy checks:
> 
> Notice: Undefined index:  general in
/opt/web/pages/trunk/extensions/Poem/Poem.php on line 37
> 
> Always test code with error_reporting set to E_ALL.
> 

OK, thanks.  That message is harmless; I've updated the patch to fix it.

> 
> I'm also a bit uneasy about these static variable; it looks like after the
> first <poem> tag, all others will behave differently because $tag is now set.

There is enough checking there that it shouldn't be a problem.  Originally, the
function just added a tag ID, but I was concerned that the strip state would
grow if every poem tag added to it, so I added the static variable, to remember
if the tag was already inserted.  If it has it, it verifies that it still has
the correct value (this is where the warning was produced, so I added the check
that it's still there and the same value).  

It it's already there, then we can just reuse the old value, rather than
duplicate it.  But if the check fails, (say, something else changed or removed
its entry) then we can ignore the old value, and reinitialize it.  

From the testing I've done so far, this seems like the best way to do it; unless
there's something else for working with the strip state that I haven't noticed.

Comment 10 Brion Vibber 2006-10-09 22:06:07 UTC
Well, why would you maintain this across separate calls which can be on separate 
Parser instances? It just doesn't many any sense to have a static variable.
Comment 11 Steve Sanbeg 2006-10-09 23:50:40 UTC
I'm expecting that adding to the strip state on each call may effectively create
a memory leak.  Do you mean that clearState() is called often enough that
anything I add will be short lived, anyway?  If so, that would simplify things.
Comment 12 Steve Sanbeg 2006-10-13 21:47:14 UTC
Created attachment 2496 [details]
new patch, no static vars.

It appears that clearState() is called frequently enough that what I was trying
to 
solve with the static variables is a non-issue, so I've removed that
functionality.
Comment 13 Steve Sanbeg 2006-10-23 16:20:38 UTC
Created attachment 2541 [details]
added credits entry, so it will display better in special;version
Comment 14 Steve Sanbeg 2006-10-23 18:44:14 UTC
Created attachment 2542 [details]
regression tests

Added another test case, to test paragraphs with leading space.  If nothing
else, at least a lot of useful regression tests are being found, bot for old
version and new enhancement.
Comment 15 Steve Sanbeg 2006-10-23 18:45:13 UTC
Created attachment 2543 [details]
new patch, with fix for paragraph with leading space.
Comment 16 Steve Sanbeg 2006-11-03 20:27:29 UTC
Created attachment 2629 [details]
Reimplemented as an extension

This extension adds the <section> hook, and two parser functions, #lst to
include a section, and #lstx to exclude.  It only needs to be included from
LocalSettings.php to be used.
Comment 17 Steve Sanbeg 2006-11-03 20:30:54 UTC
Created attachment 2630 [details]
regression tests for the extension

These are the regression tests for this extension, and show most of its
functionality.

Note that this depends on bug 7801 to run the regression test, since parser
functions currently can't be regression tested.
Comment 18 Steve Sanbeg 2006-11-03 20:37:38 UTC
Sorry, those last two attachments were misfiled, please disregard.
Comment 19 Steve Sanbeg 2006-11-22 19:57:40 UTC
Created attachment 2760 [details]
updated patch to work with Rob's changes, etc.

Update for new version.
Add Brion as author, per Nikola's request.
Use new 1.9 hook to run tests automatically.
Use method_exists instead of version_compare to detect version.
Comment 20 Steve Sanbeg 2006-11-22 19:59:20 UTC
Created attachment 2761 [details]
corrected patch.
Comment 21 Steve Sanbeg 2006-11-22 19:59:48 UTC
Created attachment 2762 [details]
update regression tests to work with Simetrical's changes
Comment 22 Steve Sanbeg 2006-12-14 21:43:31 UTC
This has been stable for awhile, so I may as well resolve it.  Committed in r18340

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


Navigation
Links