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