Hi all. We (the Wikidata team) ran into an issue recently with the value that gets passed as $baseRevId to Content::prepareSave(), see Bug 67831 [1]. This comes from WikiPage::doEditContent(), and, for core, is nearly always set to "false" (e.g. by EditPage).
We interpreted this rev ID to be the revision that is the nominal base revision of the edit, and implemented an edit conflict check based on it. Which works with the way we use doEditContent() for wikibase on wikidata, and with most stuff in core (which generally has $baseRevId = false). But as it turns out, it does not work with rollbacks: WikiPage::commitRollback sets $baseRevId to the ID of the revision we revert *to*. Now, is that correct, or is it a bug? What does "base revision" mean? The documentation of WikiPage::doEditContent() is unclear about this (yes, I wrote this method when introducing the Content class - but I copied the interface WikiPage::doEdit(), and mostly kept the code as it was). And in the code, $baseRevId is not used at all except for passing it to hooks and to Content::prepareSave - which doesn't do anything with it for any of the Content implementations in core - only in Wikibase we tried to implement a conflict check here, which should really be in WikiPage, I think. So, what *does* $baseRevId mean? If you happen to know when and why $baseRevId was introduced, please enlighten me. I can think of three possibilities: 1) It's the edit's reference revision, used to detect edit conflicts (this is how we use this in Wikibase). That is, an edit is done with respect to a specific revision, and that revision is passed back to WikiPage when saving, so a check for edit conflicts can be done as close to the actual edit as possible (ideally, in the same DB transaction). Compare bug 56849 [2]. 2) The edit's "physical parent": that would be the same as (1), unless there is a conflict that was detected early and automatic resolved by rebasing the edit. E.g. if an edit is performed based on revision 11, but revision 12 was added since, and the edit was successfully rebased, the "parent" would be 12, not 11. This is what WikiPage::doEditContent() calls $oldid, and what gets saved in rev_parent_id. Since WikiPage::doEditContent() makes the distinction between $oldid and $baseRevId, this is probably not what $baseRevId was intended to be. 3) It could be the "logical parent": this would be identical to (2), except for a rollback: if I revert revision 15 and 14 back to revision 13, the new revision's logical parent would be rev 13's parent. The idea is that you are restoring rev 13 as it was, with the same parent rev 13 had. Something like this seems to be the intention of what commitRollback() currently does, but the way it is now, the new revision would have rev 13 as its logical parent (which, for a rollback, would have identical content). So at present, what commitRollback currently does is none of the above, and I can't see how what it does makes sense. I suggest we fix it, define $baseRevId to mean what I explained under (1), and implement a "late" conflict check right in the DB transaction that updates the revision (or page) table. This might confuse some extensions though, we should double check AbuseFilter, if nothing else. Is that a good approach? Please let me know. -- daniel [1] https://bugzilla.wikimedia.org/show_bug.cgi?id=65831 [2] https://bugzilla.wikimedia.org/show_bug.cgi?id=56849 _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
