Page MenuHomePhabricator

Replace `NewRevisionFromEditComplete` with a new hook
Closed, ResolvedPublic

Description

The NewRevisionFromEditComplete hook uses Revision objects and needs to be replaced.

Suggestion for new name: RevisionFromEditComplete - all revisions are "new" if they come from an edit

Deployed extensions that would need to be updated:

  • TimedMediaHandler
  • Wikibase
  • FlaggedRevs
  • PageTriage

All of these require MW 1.35+ and thus do not need complicated version checking.

Event Timeline

Restricted Application added projects: Growth-Team, Wikidata. · View Herald TranscriptApr 16 2020, 1:51 AM
DannyS712 triaged this task as Medium priority.Apr 16 2020, 1:51 AM
DannyS712 moved this task from Unsorted to In progress on the User-DannyS712 board.

Change 589171 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Replace NewRevisionFromEditComplete with RevisionFromEditComplete

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

Change 589171 merged by jenkins-bot:
[mediawiki/core@master] Replace NewRevisionFromEditComplete with RevisionFromEditComplete

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

Change 598868 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/PageTriage@master] Replace use of NewRevisionFromEditComplete hook

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

Change 598871 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Wikibase@master] Replace use of NewRevisionFromEditComplete hook

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

Change 598872 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/TimedMediaHandler@master] Replace use of NewRevisionFromEditComplete hook

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

Change 598874 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/FlaggedRevs@master] Replace use of NewRevisionFromEditComplete hook

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

Note: the flagged revs patch needs to be merged first, because flagged revs also includes a call to run the old hook (NewRevisionFromEditComplete). Until flagged revs is updated to run both hooks, updating the other extensions means that they would no longer be triggered by flagged revs.

FlaggedRevs: https://gerrit.wikimedia.org/r/598874
PageTriage: https://gerrit.wikimedia.org/r/598868
Wikibase: https://gerrit.wikimedia.org/r/598871
TimedMediaHandler: https://gerrit.wikimedia.org/r/598872

Change 598874 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Replace use of NewRevisionFromEditComplete hook

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

DannyS712 added a subscriber: Daimona.EditedMay 27 2020, 7:23 PM

@Daimona or another phan expert - would you be willing to take a look at the wikibase phan failure?

11:56:28 <checkstyle version="6.5">
11:56:28   <file name="repo/includes/Actions/ViewEntityAction.php">
11:56:28     <error line="95" severity="warning" message="Calling method \Wikibase\Repo\Actions\ViewEntityAction::setDiffPageTitle() in \Wikibase\Repo\Actions\ViewEntityAction::overridePageMetaTags that outputs using tainted argument $[arg #2]. (Caused by: repo/includes/Actions/ViewEntityAction.php +146) (Caused by: repo/includes/Actions/ViewEntityAction.php +91)" source="SecurityCheck-DoubleEscaped"/>
11:56:28   </file>
11:56:28 </checkstyle>

The failure doesn't exist on master, and shouldn't have been caused by the commit, which doesn't touch any of the files involved in the phan error

@Daimona or another phan expert - would you be willing to take a look at the wikibase phan failure?

It's coming from an array property, so likely a false positive. Upgrading taint-check might help, perhaps it's worth trying.

I didn't investigate the failure locally, nor will I do that unless it still exists on master. Debugging taint-check failures is usually a process of trial and error that takes a lot of time.

The failure doesn't exist on master, and shouldn't have been caused by the commit, which doesn't touch any of the files involved in the phan error

It doesn't matter. Taint-check scans the code base very deeply and is often nondeterministic. The patch above may well have caused this new issue to appear.

Change 598872 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] Replace use of NewRevisionFromEditComplete hook

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

Change 598868 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Replace use of NewRevisionFromEditComplete hook

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

DannyS712 updated the task description. (Show Details)May 27 2020, 10:48 PM

Change 598871 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Replace use of NewRevisionFromEditComplete hook

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

DannyS712 changed the task status from Open to Stalled.May 28 2020, 9:32 AM
DannyS712 updated the task description. (Show Details)

All deployed extensions updated. Waiting until the current work on hooks is merged, to avoid merge conflicts and slowing that down.

Change 600401 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Hooks: Hard deprecate 2 hooks using Revision objects

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

DannyS712 changed the task status from Stalled to Open.May 30 2020, 7:08 PM

Change 600401 merged by jenkins-bot:
[mediawiki/core@master] Hooks: Hard deprecate 2 hooks using Revision objects

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

DannyS712 closed this task as Resolved.Jun 1 2020, 9:55 PM