[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Page MenuHomePhabricator

Clean up Wikibase page move notification message
Closed, ResolvedPublic5 Estimated Story Points

Description

I noticed while working on T261275 that the current code related to the “page was moved, {{WBREPONAME}} item was updated” notification message is a mess, and I believe it needs to be cleaned up. This is the timeline, as far as I can tell:

  • June 2013: First implementation of page move handling. Two hook handlers are used on the client. The “update repo” hook handler listens for a MediaWiki hook when a page was moved, and tries to schedule a job on the repo to update the connected item accordingly. The “notice” hook handler listens for a MediaWiki hook when a special page for a successful page move is being shown, and adds a notice to the HTML about the repo item being updated. The hooks communicate with one another through a dynamic property on the Title object they both receive: if the “update repo” hook handler successfully scheduled the job, it sets the wikibasePushedMoveToRepo property, and the “notice” hook handler will then choose the wikibase-after-page-move-queued message: “The item associated with this page will be automatically updated.” If the property does not exist on the Title (e. g. because the “update repo” hook handler encountered an error), then the “notice” hook handler will instead choose the wikibase-after-page-move message: “You should also update the item to maintain links.”
  • October 2015: A change in MediaWiki core causes the hook used by the “update repo” hook handler to be fired asynchronously (initially using IDatabase::onTransactionIdle; later, it will be turned into a deferred update). I believe this means that the “update repo” hook handler will now always run after the “notice” hook handler, and therefore the “notice” hook handler will never see the dynamic property added to the Title, and will always choose the wikibase-after-page-move message: “You should also update the item.”
  • June 2017: The wikibase-after-page-move message is changed from “You should also update the item” to “The move should now be reflected in the item; we ask that you check this has occurred.” – closer to the wikibase-after-page-move-queued message. The change is linked to T158842: Update wording re associated Wikidata item following move, and both the change and the task mention an “update” or “new functionality”, but I’m not aware of any changes in Wikibase that this is reacting to, and suspect it’s a misunderstanding: Wikibase has been able to update the associated item since 2013, and used to have two messages depending on whether this was successful or not; this change now changed the “unsuccessful” message into a variant of the “successful” one, because in the meantime a MediaWiki core change had caused the “successful” message to become unused, and the “unsuccessful” message to be shown always.
  • June 2020: The “update repo” hook handler is changed, and now sets the dynamic property (wikibasePushedMoveToRepo) on a newly created Title object, not the LinkTarget it received as input (which may or may not be a Title); as a result, the “notice” hook handler now has no chance to see this dynamic property even if the “update repo” hook handler somehow became synchronous again.

I see two ways forward:

  1. Fix the communication between the two hook handlers, and then add a solid test case to ensure it doesn’t break again. There is a synchronous version of the hook used by the “update repo” hook handler, PageMoveCompleting, but I’m not sure if it’s worth using. I’m also not sure if using that hook (and fixing the “June 2020” issue mentioned above) would be enough – and there’s no denying that the communication between the two hooks, by setting a random property on a MediaWiki core object that both hooks receive as input, was at best hacky even when it worked.
  1. Remove the communication and the unused message (and maybe rephrase the used message, if we want to). This means effectively no change to users – we’d just acknowledge a status quo that has existed for years, and which seems to be acceptable (I’m not aware of complaints about it).

Personally, I lean towards option 2. Showing whether the page move could be propagated to the repo or not was a nice feature when it worked, but I don’t think it’s an essential feature worth preserving with a lot of effort. (Also, even if we can enqueue the job in the repo, there’s no guarantee it doesn’t crash, so we can never have a complete guarantee that the sitelink update will work, anyways.)

Acceptance Criteria🏕️🌟 :

  • Remove the communication between the two hook handlers and remove the unused message (the second way forward as listed above)

Event Timeline

Note: currently, the “update repo” handler is the UpdateRepoHookHandler class and it listens for the PageMoveComplete hook, and the “notice” handler is the MovePageNotice class and it listens for the SpecialMovepageAfterMove hook, but I avoided mentioning those names in the timeline because this moved around somewhat over time.

Addshore subscribed.

Remove the communication and the unused message (and maybe rephrase the used message, if we want to). This means effectively no change to users – we’d just acknowledge a status quo that has existed for years, and which seems to be acceptable (I’m not aware of complaints about it).

Sounds like this would be the right choice

Addshore set the point value for this task to 5.

There is a basically exact same situation with wikibase-after-page-delete-queued as well. I have to check the hook handlers to be sure but if you know on top of your head that it's the case or not. Let me know.

Change 709489 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/Wikibase@master] Drop wikibase-after-page-move-queued message

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

There is a basically exact same situation with wikibase-after-page-delete-queued as well. I have to check the hook handlers to be sure but if you know on top of your head that it's the case or not. Let me know.

Doesn’t ring a bell, please check :)

There is a basically exact same situation with wikibase-after-page-delete-queued as well. I have to check the hook handlers to be sure but if you know on top of your head that it's the case or not. Let me know.

Doesn’t ring a bell, please check :)

I'll check that separately. The first patch is ready for review.

Change 709489 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Drop wikibase-after-page-move-queued message

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

No the delete hook (ArticleDeleteCompleteHook) is synchronous as far as I can see.