It looks like we should leave the existing hook parameters values alone for the moment, but it would improve the situation if we renamed variables which seem to be overloaded or unclear, in MediaWiki core and in FlaggedRevs. What do you think of the following conventions,
'oldid' (index.php parameter) -- keep this name only to preserve interface compatibility. This refers to a historical revision when used in the action=view case, and to the latest revision ID of the page at the time an edit session begins. $oldid -- keep as-is in the action=view codepath, rename to $parentRevId in action=edit $parentRevId -- latest available revision ID at the time an edit session begins. Used to detect conflicts, and identify the parent revision record upon save. This is updated during successful automatic rebase. I don't see a good use case for preserving what Daniel calls the "reference revision," the parentRevId before rebase. $baseRevId and $baseId -- rename everywhere to $contentsRevId, but examining the code contexts for the smell of confounding with $parentRevId. $contentsRevId -- revision ID of the source text to copy when performing undo or rollback. We will probably want to supplement hooks that only passed $contentsRevId, such as NewRevisionFromEditComplete, with $parentRevId as an additional parameter. A refactor along these lines would keep me from losing already scant marbles as I attempt to fix related issues in core: https://gerrit.wikimedia.org/r/#/c/94584/ , I see now that I've already begun to introduce mistakes caused by the difficult common-sense interpretation of current variable naming. -Adam On Mon, Jun 2, 2014 at 1:22 AM, Daniel Kinzler <dan...@brightbyte.de> wrote: > Am 30.05.2014 15:38, schrieb Brad Jorsch (Anomie): > > I think you need to look again into how FlaggedRevs uses it, without the > > preconceptions you're bringing in from the way you first interpreted the > > name of the variable. The current behavior makes perfect sense for that > > specific use case. Neither of your proposals would work for FlaggedRevs. > > As far as I understand the rather complex FlaggedRevs.hooks.php code, it > assumes > that > > a) if $newRev === $baseRevId, it's a null edit. As far as I can see, this > does > not work, since $baseRevId will be null for a null edit (and all other > regular > edits). > > b) if $newRev !== $baseRevId but the new rev's hash is the same as the base > rev's hash, it's a rollback. This works with the current implementation of > commitRollback(), but does not for manual reverts or trivial undos. > > So, FlaggedRevs assumes that EditPage resp WikiPage set $baseRevId to the > edits > logical parent (basically, the revision the user loaded when starting to > edit). > That's what I described as option (3) in my earlier mail, except for the > rollback case; It would be fined with me to use the target rev as the base > for > rollbacks, as is currently done. > > FlaggedRevs.hooks.php also injects a baseRevId form field and uses it in > some > cases, adding to the confusion. > > In order to handle manual reverts and null edits consistently, EditPage > should > probably have a base revision as a form field, and pass it on to > doEditContent. > As far as I can tell, this would work with the current code in FlaggedRevs. > > > As for the EditPage code path, note that it has already done edit > conflict > > resolution so "base revision = current revision of the page". Which is > > probably the intended meaning of false. > > Right. If that's the case though, WikiPage::doEditContent should probably > set > $baseRevId = $oldid, before passing it to the hooks. > > Without changing core, it seems that there is no way to implement a > late/strict > conflict check based on the base rev id. That would need an additional > "anchor > revision" for checking. > > The easiest solution for the current situation is to simply drop the strict > conflict check in Wikibase and accept a race condition that may cause a > revision > to be silently overwritten, as is currently the case in core. > > -- daniel > > > _______________________________________________ > Wikitech-l mailing list > Wikitech-l@lists.wikimedia.org > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
