Page MenuHomePhabricator

Make RevisionRecord::getPageId() take a wiki ID
Open, HighPublic

Description

Context:
When creating PageIdentity (T208776) we decided that the getId() method should require a wiki ID to be passed in if the page belongs to a wiki other than the local site. The same patter should be applied to all getters on entity objects that return an ID that is likely to be used as a foreign key in a database table, to avoid data corruption.

After doing this for RevisionRecord::getId() (T272485), we should also do it for RevisionRecord::getPageId().

Note that the change to the method signature is not a breaking one, since the parameter would be optional. However, it's a breaking change in behavior: code that loads a RevisionRecord from another wiki and then calls getPageId() on it will need to be updated. So this change needs to go through the deprecation process.

  • make RevisionRecord::getPageId() trigger a deprecation warning when called without parameters, if it doesn't belong to the local wiki ($this->mWikiId is not false).
  • make RevisionRecord's trigger a deprecation warning when $wikiId does not match the $page parameter's wiki.
  • make RevisionRecord::getPageId() throw an exception if the $wikiId parameter does not match the $this->mWikiId field. Compare RevisionRecord::getId().
  • make RevisionRecord's constructor throw an exception when $wikiId does not match the $page parameter's wiki.

Event Timeline

daniel raised the priority of this task from Medium to High.Feb 3 2021, 5:45 PM

Change 662130 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] Make RevisionRecord::getPageId() and :: getParentId take a wiki ID

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

Change 662130 merged by jenkins-bot:
[mediawiki/core@master] Make RevisionRecord::getPageId() and :: getParentId take a wiki ID

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

Now we need to update RevisionStore to pass in correct $wikiId to both methods.

Change 663314 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] Make RevisionRecord::getPageId() take a wiki ID

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

Change 663314 merged by jenkins-bot:
[mediawiki/core@master] Make RevisionStore to pass $wikiId to RevisionRecord's methods.

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

We are throwing deprecation warnings now. Need to wait until starting to throw

Change 672462 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] RevisionStore: getRevisionbyId should take a PageIdentity

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

Change 672499 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] RevisionStore: create getPage, deprecate getTitle

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

Change 672819 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Re-apply "Deprecate constructing revision with non-proper page"

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

Back to "review" for

When this is done, this should go to "waiting for release", since we can only start throwing errors after 1.36 has been branched.

Change 672499 merged by jenkins-bot:
[mediawiki/core@master] RevisionStore: create getPage, deprecate getTitle

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

Change 672462 merged by jenkins-bot:
[mediawiki/core@master] RevisionStore: getRevisionbyId should take a PageIdentity

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

Change 672819 merged by jenkins-bot:
[mediawiki/core@master] Re-apply "Deprecate constructing revision with non-proper page"

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

@daniel: Per emails from Sep18 and Oct20 and https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup , I am resetting the assignee of this task because there has not been progress lately (please correct me if I am wrong!). Resetting the assignee avoids the impression that somebody is already working on this task. It also allows others to potentially work towards fixing this task. Please claim this task again when you plan to work on it (via Add Action...Assign / Claim in the dropdown menu) - it would be welcome. Thanks for your understanding!