Change Details

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](https://gerrit.wikimedia.org/r/65648): 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](https://gerrit.wikimedia.org/r/244044): 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](https://gerrit.wikimedia.org/r/357979): 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}, 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](https://gerrit.wikimedia.org/r/608111): 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.~~ 2. 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)

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](https://gerrit.wikimedia.org/r/65648): 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](https://gerrit.wikimedia.org/r/244044): 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](https://gerrit.wikimedia.org/r/357979): 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}, 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](https://gerrit.wikimedia.org/r/608111): 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.~~ 2. 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)

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](https://gerrit.wikimedia.org/r/65648): 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](https://gerrit.wikimedia.org/r/244044): 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](https://gerrit.wikimedia.org/r/357979): 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}, 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](https://gerrit.wikimedia.org/r/608111): 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.~~ 2. 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)