Page MenuHomePhabricator

Use alternative storage to store Variables data currently stored in Parser
Open, HighPublic8 Estimated Story Points

Event Timeline

MGChecker renamed this task from Use ParserOutput to store Variablesdata currently stored in Parser to Use ParserOutput to store Variables data currently stored in Parser.Sep 5 2018, 3:02 AM
MGChecker created this task.

Change 468682 had a related patch set uploaded (by MGChecker; owner: MGChecker):
[mediawiki/extensions/Variables@master] Use ParserOutput to store Variables data currently stored in Parser

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

MGChecker set the point value for this task to 8.Oct 25 2018, 12:05 AM

Change 468682 had a related patch set uploaded (by MGChecker; owner: MGChecker):
[mediawiki/extensions/Variables@master] Use ParserOutput to store Variables data currently stored in Parser

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

MGChecker claimed this task.
MGChecker edited projects, added Patch-For-Review; removed Patch-Needs-Improvement.

This has become a bit more pressing with dynamic properties being deprecated with PHP 8.2. While MediaWiki is not compatible with that yet, it is only a matter of time. In any case, we will need to fight around T300979 and T343227 soon I guess.

I am not sure if this is the right way forward: these stored as extension data but would never be used after parsing while they are in the ParserOutput and ParserCache.

If we only consider the Variables extension itself, we can use an instance for Parser::setFunctionHook() calls, so it can carry the data along the parsing.
But the difficult part is that other extensions need to interact with Variables. While they can still use the Parser::callParserFunction() method, there are extra overheads compared to calling the ExtVariables::setVarValue() method directly.

Considering our discussions about this, let's remove this from the 2.6.0 milestone.

If we only consider the Variables extension itself, we can use an instance for Parser::setFunctionHook() calls, so it can carry the data along the parsing.
But the difficult part is that other extensions need to interact with Variables. While they can still use the Parser::callParserFunction() method, there are extra overheads compared to calling the ExtVariables::setVarValue() method directly.

According to the new progress of T343227, we may have to go with this route since the ParserOutput will be unreadable during parsing in the future.

We may introduce a new hook in the Variables extension for other extensions to interact with, providing two parameters, the Parser and the ExtVariables instance. Extensions handling this hook can store a reference of the variables store, so they can call setVarValue() directly later.

Func renamed this task from Use ParserOutput to store Variables data currently stored in Parser to Use alternative storage to store Variables data currently stored in Parser.Feb 18 2024, 8:23 AM

At the moment, I still aim to get a stance in T282499. If they decline it and parallelize parsing entirely, we can keep Variables more or less running only so long. Otherwise, they should include a dedicated mechanism for our use case.

On the Gerrit changeset, @cscott has indicated his willingness to declare mVariables explicitly in the parser for now, overriding the deprecation. Therefore, there would be no dire need to go away from this pattern.
I am hestitant towards making the type of breaking change as you suggest, as there is not guarantee it would be a long-term solution.

I would not rely on T282499: Consider whether Parsoid will support forced linear parsing. -- there's no consensus on that path, and even if compatibility modes are supported, they will likely not be used in production by the foundation, and as a result you can expect them to be unmaintained. There's been a patch attached to this task since 2018, you should move forward with that.

Similarly, as discussed in T343227: Remove #[AllowDynamicProperties] from Parser class the long-term plan is certainly to deprecate use of the Parser object to store persistent state across a parsed page. It would be better to see proactive effort to address these issues while there is less time pressure and sense of urgency, rather than to wait until the last minute.

@cscott The patch in question also makes use of linear parsing, but is using the ParserOutput extension data to do so rather than the parser object itself.
It is not clear whether read-write access to ParserOutput extension data or the properties in the Parser are going away earlier, so the migration path to maintain compatibility as long as possible is not clear.

Some adjusted notions of Variables have been discussed, that make less use of the order of processing. But without any knowledge how the end result for the parser will look like, it does not seem wise to actively work in a certain direction just to have all efforts in vain in the end.

For Wikis that use Variables just for caching purposes, an interface to specify the information in a separate slot for the revisions, where it can then be queried from, seems expedient. However, building something like this is over the top of my abilities.

I am aware of the bleak future ahead. My personal mission here is to keep this extension running as long as reasonably possible. We do communicate that its future is uncertain either way.