How would such a magic word avoid the need to disable caching / fragmenting renders between previews & actual view?

Could we instead discourage this kind of behavior change on preview? What are the use cases that drive this, and can they be supported more sanely?

We don't share the renders, so that's not an issue. Stashing/doEditContent/doEditUpdates all happen in non-preview mode. The preview output is not cached anywhere.

Stashing/doEditContent/doEditUpdates all happen in non-preview mode.

I see, that makes this less of a concern for that particular case.

I am still doubtful about introducing a new parser function. VisualEditor and Parsoid probably already get a mix of preview & normal render content, depending on whether users switched from wikitext or not. A new parser function could be clamped to always return false in that context, but then people will probably just continue to look at {{REVISIONID}}, thereby not really solving the problem. And, we'd have another not-quite-working parser function to maintain.

In T137900#2384121, @GWicke wrote:

Could we instead discourage this kind of behavior change on preview? What are the use cases that drive this, and can they be supported more sanely?

Lua modules use this to insert debug messages into the preview HTML. Take a look at Module:Check for unknown parameters to see what I mean.

It is indeed an abuse of preview, because the intended purpose of preview is to, well, pre-view how the page would look like if the staged editor were saved. Preview-specific HTML defeats that.

Possible solutions:

• Don't vary the output at all, and instead rely on CSS to show / hide these messages. Possibly a non-starter if it causes debug messages to show up in search results.
• Don't vary the output at all, and instead use JavaScript to scrape and display specially-marked <!-- comments --> when previewing.
• Don't vary the output at all, and provide an API for Lua to log messages that are saved in ParserOutput but not as part of the HTML.
• Just explain that preview warnings are an abuse of previews and try to persuade editors to stop using them. This is my favorite option, since I question the value and sanity of these debug messages. But I don't think it would go over well with editors. I could be wrong, though.

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

In T137900#2384230, @Legoktm wrote:

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

Yes, this seems like a better solution.

Change 294774 had a related patch set uploaded (by Aaron Schulz):
[WIP] Disable expensive REVISION* magic words in miser mode

https://gerrit.wikimedia.org/r/294774

In T137900#2384230, @Legoktm wrote:

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

These don't have locational context, but I'd agree that they'd be useful for some things. Errors like "invalid param 'foo'" would just mean the user has to cntrl-f for 'foo' to find that parameter.

Change 294776 had a related patch set uploaded (by Legoktm):
Expose ParserOutput::addWarning() to modules

https://gerrit.wikimedia.org/r/294776

In T137900#2386640, @aaron wrote:

In T137900#2384230, @Legoktm wrote:

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

These don't have locational context, but I'd agree that they'd be useful for some things. Errors like "invalid param 'foo'" would just mean the user has to cntrl-f for 'foo' to find that parameter.

I'm pretty sure that the lack of locational context would keep enwiki from switching to this.

In T137900#2386686, @Jackmcbarn wrote:

I'm pretty sure that the lack of locational context would keep enwiki from switching to this.

I don't see why. I see how locational context is useful, but performance is important too, and so is having preview functionality that is faithful to the final HTML.

Change 294776 merged by jenkins-bot:
Expose ParserOutput::addWarning() to modules

https://gerrit.wikimedia.org/r/294776

I would personally think of these templates emitting these errors during preview as a different kind of edit notice, which have been implemented in the software for a while.

Most of these templates do have a tracking category, or even emit visible errors (CS1 does, at least) into the page content. What these sometimes-complex templates are trying to prevent is adding another page edit to fix some error that could have been caught in preview.

Some of this problem goes away when editors aren't touching the source directly as with VisualEditor ("wrong/old/deprecated parameters"), but other errors are emitted due to the value that the parameter takes and not the parameter name itself ("check ISBN for goodness").

Personally, I think a patch that permits a module to access the API to determine whether a page is in preview mode is a good thing, due to those use cases. I'm fairly certain a magic word is unnecessary, but would not be opposed to it.

Change 295109 had a related patch set uploaded (by Aaron Schulz):
[WIP] Try to predict the rev_id when preparing edits

https://gerrit.wikimedia.org/r/295109

Change 294642 abandoned by Aaron Schulz:
[WIP] Add {{PREVIEWMODE}} magic word

https://gerrit.wikimedia.org/r/294642

Change 295109 merged by jenkins-bot:
Try to predict the rev_id when preparing edits

https://gerrit.wikimedia.org/r/295109

Change 296794 had a related patch set uploaded (by Aaron Schulz):
Try to predict the rev_id when preparing edits

https://gerrit.wikimedia.org/r/296794

Change 296794 merged by jenkins-bot:
Try to predict the rev_id when preparing edits

https://gerrit.wikimedia.org/r/296794

These "preview-only" warnings are causing Parsoid render issues like this:

/cc @ssastry @tstarling

In T137900#2413628, @gerritbot wrote:

Change 294642 abandoned by Aaron Schulz:
[WIP] Add {{PREVIEWMODE}} magic word

https://gerrit.wikimedia.org/r/294642

@Krinkle wrote at https://gerrit.wikimedia.org/r/470099 (the wrong Gerrit patch, oops):

A visual diffing job has been started at http://mw-expt-tests.wmflabs.org/ - it has two VMs with MediaWiki-Vagrant with a few dozen wikis for different projects/languages, and 64547 articles imported. The "expt" VM has this patch applied.

The label for this job is "FIX_REVID".

64,547 articles were tested. Of these 3 rendered differently between the two versions of MediaWiki. I've confirmed them all to be false negatives;

• frwiki / Tiyi-Meryaset: Use of time-based parser function to show a different "random" article in the navigation template.
• ruwiki / ФК «Зенит» Санкт-Петербург в сезоне 2014/2015: Use of text that varies by the number of seconds on the clock. After rendering them a second time in the same second, they became identical.
• frwiki / Pyramide_de_Sésostris_Ier: Another navigation template picking a "random" article.

So.. looks like we're good :)

Change 294774 merged by jenkins-bot:
[mediawiki/core@master] Disable expensive {{REVISIONID}} magic word in miser mode

https://gerrit.wikimedia.org/r/294774

Based on a conversation with @Anomie on IRC, we've identified that {{REVISIONID}} is used across templates in the projects for things such as inserting the permanent link number automatically as log reason (list of templates). Having the templates insert automatically the oldid comes very handy. Will we have a replacement for this function? Thanks.

Edit: Nevermind. My mistake.

@Johan

The magic word {{REVISIONID}} is being deprecated for performance reasons. In the future, it will only return "" (empty string) when previewing edits, or "-" (dash) when reading pages. The release of next week, will only change this behaviour for articles in the content namespaces. It will not change for interface messages and other namespaces (such as talk pages, and user pages). T137900

What about {{REVISIONTIMESTAMP}}?

In T137900#5087502, @geraki wrote:

What about {{REVISIONTIMESTAMP}}?

I wouldn't add any uses of that, and ideally things should move away from it. Nevertheless, nothing about that word has changed, only REVISIONID for the moment.

@Krinkle Thanks, very helpful.

Now every time an edit is made to an article where there are unknown parameters, the preview-only warning is shown on view until the page is purged; see report of issues at VPT.

In T137900#5107898, @Galobtter wrote:

Now every time an edit is made to an article where there are unknown parameters, the preview-only warning is shown on view until the page is purged; see report of issues at VPT.

Correct. You should fix the hacky Lua relying on the former behaviour.

In T137900#5107955, @Jdforrester-WMF wrote:

In T137900#5107898, @Galobtter wrote:

Now every time an edit is made to an article where there are unknown parameters, the preview-only warning is shown on view until the page is purged; see report of issues at VPT.

Correct. You should fix the hacky Lua relying on the former behaviour.

I thought the Lua module was the problem as well, e.g. checking it to be a number or something like that. But it's not. The Lua module is doing the right thing, by checking {{REVISIONID}} against the empty string ''.

The part we have removed as part of this task is that on regular views it is now the dash - instead of a number. Equality to the empty string is still the supported and only way to detect preview mode.

Change 527125 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] parser: further restrict "revisionid" by namespace in $wgMiserMode

https://gerrit.wikimedia.org/r/527125

Further roll out tracked at T235957.