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 <daniel(a)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(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l