Page MenuHomePhabricator

ParserOptions' currentRevisionCallback uses Revision objects
Closed, ResolvedPublic

Description

ParserOptions has a callable variable, currentRevisionCallback, that is set and retrieved via ::getCurrentRevisionCallback and ::setCurrentRevisionCallback. The callbacks uses return Revision objects, which are being deprecated. However, no alternative currently exists to replace the uses.

Event Timeline

DannyS712 triaged this task as Medium priority.Apr 3 2020, 10:28 PM
DannyS712 created this task.
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.

I did some more digging:
The callback is for use in Parser::fetchCurrentRevisionOfTitle

DannyS712 added a comment.EditedApr 4 2020, 1:08 AM

The extensions that use either of these:

  • fetchCurrentRevisionOfTitle is used in Scribunto and TemplateStyles
  • statelessFetchRevision (the default callback) is only used in core
  • getCurrentRevisionCallback and setCurrentRevisionCallback are only used in TemplateStyles, FlaggedRevs, TemplateSandbox, and Parsoid

Compatibility policies:

  • Scribunto: 1.35+
  • TemplateStyles: MW 1.31+
  • FlaggedRevs: MW 1.35+
  • TemplateSandbox: MW 1.33+
  • Parsoid: MW ^1.34 (i.e. doesn't support current version and can be ignored)

In short, 4 extensions need to be updated, and 2 updates need to be version dependent (i.e. behaviour switch based on version) to be able to hard deprecate the old system in 1.35

DannyS712 added a comment.EditedApr 9 2020, 3:28 AM

I started looking into this, and found that:

Documentation for Revision::newFromRow says MCR migration note: replaced by RevisionStore::newRevisionFromRow(). Note that newFromRow() also accepts arrays, while newRevisionFromRow() does not. Instead, a MutableRevisionRecord should be constructed directly. RevisionStore::newMutableRevisionFromArray() can be used as a temporary replacement, but should be avoided.

Documentation for MutableRevisionRecord::__construct says @note Avoid calling this constructor directly. Use the appropriate methods in RevisionStore instead.

The relevant documentation in for both methods was added by @daniel in T174025: Revision::newFromRow had the note added in https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/6af796f3e0cf3e66cd7d7e59af8445f5712d68fe%5E%21/#F2 and MutableRevisionRecord::__construct had the note added in https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/e61a1caaddb58cc26bf5f912940afbb2a6f65355%5E%21/#F4 when the class was originally created.

Given that the replacement here needs to be able to return a RevisionRecord that is not known to be in the database, MutableRevisionRecord seems like the ideal object, but its unclear how it should be created. @daniel, can you clarify how the object should be created?

^ I'm not sure why I thought a MutableRevisionRecord was needed, done differently (but the correct way to create it should still be clarified)

Change 587627 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add RevisionRecord alternatives to Parser and ParserOptions methods

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

Change 587627 merged by jenkins-bot:
[mediawiki/core@master] Add RevisionRecord alternatives to Parser and ParserOptions methods

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

Change 589644 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/TemplateStyles@master] Use RevisionRecordCallback in ParserOptions in MW 1.35+

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

^ I'm not sure why I thought a MutableRevisionRecord was needed, done differently (but the correct way to create it should still be clarified)

Oh, its needed when the extension wants to pass a RevisionRecord with the fields specified; should it construct the MutableRevisionRecord manually or via RevisionStore?

Change 589646 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Follow up I66cbcb963a96cc49c75ca72faa7e439ae6d6614d - update release notes

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

Change 589646 merged by jenkins-bot:
[mediawiki/core@master] Follow up I66cbcb963a96cc49c75ca72faa7e439ae6d6614d - update release notes

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

Change 589693 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/FlaggedRevs@master] Use RevisionRecordCallback in ParserOptions

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

Change 589697 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/TemplateSandbox@master] Use RevisionRecordCallback in ParserOptions in MW 1.35+

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

Change 589699 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Scribunto@master] Remove use of Parser::fetchCurrentRevisionOfTitle

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

Change 589702 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Replace uses and hard deprecate Parser::getRevisionObject

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

DannyS712 moved this task from Next to In progress on the User-DannyS712 board.Apr 27 2020, 7:13 AM

Change 589699 merged by jenkins-bot:
[mediawiki/extensions/Scribunto@master] Remove use of Parser::fetchCurrentRevisionOfTitle and Revision::getSha1

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

Change 589702 merged by jenkins-bot:
[mediawiki/core@master] Replace uses and hard deprecate Parser::getRevisionObject

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

@cscott would you mind taking a look at the patches I submitted for the extensions? Since you are familiar with this part of the code and gave the go-ahead to the new implementation at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/587627/

Change 589693 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Use RevisionRecordCallback in ParserOptions

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

Change 601935 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/TemplateStyles@master] Remove use of Parser::fetchCurrentRevisionOfTitle

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

Change 589697 merged by jenkins-bot:
[mediawiki/extensions/TemplateSandbox@master] Use RevisionRecordCallback in ParserOptions, require MW 1.35+

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

Change 589644 merged by jenkins-bot:
[mediawiki/extensions/TemplateStyles@master] Use RevisionRecordCallback in ParserOptions, require MW 1.35+

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

Change 601935 merged by jenkins-bot:
[mediawiki/extensions/TemplateStyles@master] Remove use of Parser::fetchCurrentRevisionOfTitle

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

Change 601941 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Replace core uses and hard deprecate Parser(Options) Revision methods

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

Change 601941 merged by jenkins-bot:
[mediawiki/core@master] Replace core uses and hard deprecate Parser(Options) Revision methods

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

DannyS712 closed this task as Resolved.Jun 4 2020, 2:13 AM