Page MenuHomePhabricator

DOM preprocessor barfs on headings inserted by parser functions
Open, LowPublic

Description

Author: jpatokal

Description:
Ensure that the title attribute is set before trying to add in the heading index marker

If a parser function extension inserts a heading into an article, PPFrame_DOM::expand() chokes on it because its title attribute is not set:

1041 $titleText = $this->title->getPrefixedDBkey();

Since title attributes are only required for inserting heading index markers, and these markers should most probably not be inserted for headings not within the article content, the simple fix is to check that the title attribute is set before trying to add in the heading index marker. A trivial patch is attached.


Version: 1.16.x
Severity: minor

Attached:

Details

Reference
bz21844

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:52 PM
bzimport set Reference to bz21844.
bzimport added a subscriber: Unknown Object (MLST).

Proposed fix: Check also for null values of $title in PPFrame_*::newChild()

The title attribute of a frame should always be set. It's not working in your case, because there is a little bug in PPFrame_DOM::newChild(), checking only for the value false but not null.

It would be helpful if you could try the attached patch with your code and tell us, if it works for you.

Attached:

jpatokal wrote:

Thanks for the fast response. The patch works in the sense of not crashing anymore, but the autogenerated edit links now all point to nonexistent sections of incorrect articles. Eg. in the article "Main":

Orientation== [edit] -> http://atlas-mediawiki.local/index.php?title=Main&action=edit&section=1

Information== [edit] -> http://atlas-mediawiki.local/index.php?title=Orientation&action=edit&section=T-1

Simply disabling section editing entirely and generating my own edit links via the extension is probably an acceptable workaround though.

(In reply to comment #2)

Thanks for the fast response. The patch works in the sense of not crashing
anymore, but the autogenerated edit links now all point to nonexistent sections
of incorrect articles. Eg. in the article "Main":

Orientation== [edit] ->

http://atlas-mediawiki.local/index.php?title=Main&action=edit&section=1

Information== [edit] ->

http://atlas-mediawiki.local/index.php?title=Orientation&action=edit&section=T-1

You should be able to specify the correct title in the array returned by your parser function:

return array( $output, 'noparse' => false, 'title' => $yourTitleObject );

The section indices might be wrong, though, so I'm not sure, if this is what you're actually trying to do.

Simply disabling section editing entirely and generating my own edit links via
the extension is probably an acceptable workaround though.

You can generate headings without edit links via

<h2> Section title </h2>

jpatokal wrote:

Thanks. On second thought, the edit links are another kettle of fish, and the original issue I raised is fixed by your patch -- please add it to the trunk and close this bug as FIXED.

Please provide sample calling code which allows me to confirm and reproduce this issue.

I don't know if he is still around, but I was able to reproduce it with something like:

$wgHooks['ParserFirstCallInit'][] = 'efBug21844_setHook';
$wgHooks['LanguageGetMagic'][] = 'efBug21844_setMagic';
function efBug21844_setHook( $parser ) { $parser->setFunctionHook( 'bug21844', 'efBug21844' ); return true; }
function efBug21844_setMagic( $magicWords ) { $magicWords['bug21844'] = array( 1, 'bug21844' ); return true; }
function efBug21844() {
return array( "== Foo ==\n...", 'noparse' => false );
}

and {{#bug21844:}} on any page.

jpatokal wrote:

Still around, and you beat me to it!

The problem is the fact that at Parser.php line 2900 (r60825), $isChildObj is set without $title being set. This is incorrect, you can't have a child object with no title. This causes line 2995 to create an invalid frame with $title=null, which then throws fatal errors under various circumstances.

This behaviour of $noparse was apparently added to support DPL2 in r34594, although DPL2 has since stopped using it. I'm not sure if it set $title. Theoretically LST could use it (if it set $title correctly) but it doesn't.

It doesn't really make sense to use $isLocalObj, since that would give you non-functional edit section links for the current page. If you were getting text from another page and including it like a template, then $isChildObj could make sense, since it would give you edit section links to the other page, if you set $title.

If the text doesn't come from an editable page at all, then we probably need to have a flag which can be sent to expand() to suppress the generation of heading markers.

So, Jani, can you please explain your application, particularly what sort of text it is that you're trying to expand? And in that text, what do you expect triple-braces to do?

jpatokal wrote:

I'm not entirely sure why my application has relevance on the actual bug, which is that inserting titles by parser function fails...?

But at any rate, the parser function is called #include and it's used with the format {{#include:Article}}. Depending on page state it either returns a link to Article:

  • [[Article]]

...or it asks the parser to transclude it, with the article name added as a header:

Article

{{:Article}}

...and it's the 2nd case here that's failing.

sumanah wrote:

Adding the "reviewed" keyword since Tim reviewed the author's approach. P.Copp, Jani, if you would like more feedback, please feel free to replace that keyword with "need-review". Thanks.