Page MenuHomePhabricator

Clean up ApiEditPage content handler undo logic
Closed, DeclinedPublic

Description

Sorry for the clunky title.
Currently, ApiEditPage specifies that

if ( $params['undo'] > 0 ) {
	// allow undo via api
} elseif ( $contentHandler->supportsDirectApiEditing() === false ) {
	$this->dieWithError( [ 'apierror-no-direct-editing', $model, $name ] );
}

instead of having an empty if statement, the two conditions should just be merged. Patch to be uploaded shortly

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 530786 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Clean up ApiEditPage content handler undo logic

https://gerrit.wikimedia.org/r/530786

I wouldn't really call this "tech debt". Code is arranged like that in order to clearly state that undos are allowed, and that the two conditions (undo check and contenthandler check) are not related. If we really want to merge them into a single statement, I believe we should at least re-add a comment stating that undos are always allowed. Otherwise, it might not be clear why we're checking for that. But again, I'm unsure whether this cannot be left as-is.

I've restored the comment to make it clear that undos are always allowed

Change 530786 merged by jenkins-bot:
[mediawiki/core@master] Clean up ApiEditPage content handler undo logic

https://gerrit.wikimedia.org/r/530786

Change 530869 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Revert "Clean up ApiEditPage content handler undo logic"

https://gerrit.wikimedia.org/r/530869

Anomie added a subscriber: Anomie.

Reverted and reopened, and I recommend declining instead. I find the new code far less clear as to intention than the code with the empty if case. Copying from my revert's commit summary:

I find the empty case far clearer as to intention than this change, and I doubt this is such a hot path that optimization should take precedence over clarity. If we really want to get rid of the empty case, I'd have structured it somewhat clearly as

if (
    // always allow undo via api, T230702
    !( $params['undo'] > 0 ) &&
    // [something else here to explain this clause]
    $contentHandler->supportsDirectApiEditing() === false
) {

But even that is IMO less clear. At the very least this seems to deserve more discussion.

Change 530869 merged by jenkins-bot:
[mediawiki/core@master] Revert "Clean up ApiEditPage content handler undo logic"

https://gerrit.wikimedia.org/r/530869

Marking as declined per T230702#5422040 - I was just trying to clean up the logic, but it seems that there is a benefit to the current structure