r79520 MediaWiki - Code Review archive

Seen on Translatewiki:

 PHP Catchable fatal error: Argument 1 passed to Linker::doEditSectionLink() must be an instance of Title, null given in /www/w/includes/Linker.php on line 1383

Should be fixed now. The parser was modifying the data inside the marker causing that bug, switched to outputting the data in a way that shouldn't be touched by the parser.

I'm not convinced by the $wgUseEditSectionTokens. I think it could be made with a nicer fallback.

Dont forget to fix the parser tests you broke.

Should we revert now r70940 ?

I really really dislike embedding this data in the wikitext output. Can't we store it as a variable in ParserOutput, keyed to the id of the section?

Sounds good. You'd still have to insert the UNIQ string though, except it'd just contain the key into the data array.

We go to so much trouble generating a unique HTML id for each section header for the ToC, why not just use that?

Because it might occur in other locations in the wikitext too? In this case, it's actually pretty likely to (if it's pure ASCII and doesn't contain spaces). UNIQ-blahblah-QINU markers, however, are so crazy and random that they're practically never gonna produce collisions with actual content.

Even worse, the LanguageConverter completely screws everything over. It converts UNIQ to things like УНИQ and completely breaks the edit section tokens. And it expects certain parts of the edit token to be language transcoded, while expecting other parts to be left untouched. And the only solution seams to be to encode things in a <editsection page="Title" section="1">Text</editsection> format so that the LanguageConverter will properly convert "Text" but will not touch the page title, section hint, and actual editsection markers.

And just since someone is probably going to make a comment on it.

Tags are already escaped at this point in time, so using <editsection> in the output doesn't cause any issue with user content, '<editsection page="Title" section="1">Text</editsection>' in an article will not be turned into an arbitrary editsection token unless you actually create a tag hook to do that for you. It's even safer than the UNIQ tokens which have the (even though it's insanely remote) chance that the random part of the marker might be a match if a user tries to insert some arbitrary text in the same format as the UNIQ tokens.

If the LanguageConverter is hacking at HTML IDs, that's a bug; surely that's going to break css all over the place? But the best way to 'protect' this content from it is not to include it in page text at all. It's not unreasonable for the Converter to assume that anything in the text output is fair game for conversion; the problem is arising from inserting material into the text which is not text.

No, it's hacking away at the UNIQ tags left in the page. The LanguageConverter has some intelligent handling for html tags, it translates content, and some attributes like title, but leaves tag names and the rest of the attributes alone.

The other issue is that we actually have to let the LanguageConverter do transcoding of some of the editsection, namely allowing it to transcode the headline hint. Otherwise we break parser tests.

Exactly, if you didn't use UNIQ tags to embed this stuff, you wouldn't be fighting with the Converter. You're trying to reuse various parser structures in a novel context, and finding that other elements of the parser are unhappy because they don't expect those elements to be there at the point when they run. I'm saying you shouldn't be altering the text output at all, you should be looking at what's in the output anyway that you can use to find sections (eg the HTML IDs), and store the necessary data elsewhere in ParserOutput, where other bits of the parser ecosystem aren't going to attack it. Surely you can call the LanguageConverter separately on the bits of text you need converted?

Ideally, something like this would be implemented entirely with client-side JS; failing that, a server-side postprocessing step. Basically, you're correct to want it out of the parser text output. Why not take it all the way out?

LanguageConverter avoid converting UNIQ stuffs and HTML attrbutes with some regular expressions in autoConvert(). It's not the best solution but still effective.

Instead of using $wgUseEditSectionTokens, you should just bump the parser version. That'll cause any cached ParserOutputs generated by the old version to automatically be considered invalid.

I already killed $wgUseEditSectionTokens in r79568

Can you please rephrase the RELEASE NOTE entry. It is a five lines sentence!

Ok, done.

I've done something similar to this before, for embedding dynamic LiquidThreads pages into other pages with wiki markup.

Given how that went for me, I'd recommend the following:

• Use an HTML comment with a prefix and a random hexadecimal string instead of parser strip markers. This won't be touched by the language converter.
• Generate the section edit links at parse time instead of at replace time, and store them in the Parser Output as a straight associative array of string to string replacements. Then, in OutputPage::addHTML or thereabouts, figure out whether you're going to show them or not, and either do the replacement or strip them out.
• There's no need to purge the parser cache en-masse. I'm not quite sure how you'd factor it, but if the appropriate entry is available in the parser cache with section edit links substituted in, then in theory there's no need to regenerate the parser output with them optional if the user wants section edit links.

• Editsection tokens DO need to be partially touched by the LanguageConverter, if specific parts aren't touched by the LC we introduce a regression
• We don't want to generate edit section links at parse time because edit section links are rendered in the user's lang not the content lang, and we don't want to re-render the same page for each different user lang just because a piece of the ui we could defer has to be re-rendered.
• We replace stuff inside ParserOutput::getText because it's the common api, we're just trying to defer editsection link rendering, the OutputPage::addHTML area is only one of the possible places we may grab the ParserOutput's text, if we only do the replacements there we'll introduce a regression where we need to make the same code refactoring to other places, and potentially even inside extension code.
• Stuff to do with the parser cache has already been handled in followup commits.

The parser cache mass-purge was fixed by r79568