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
Change 530786 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Clean up ApiEditPage content handler undo logic
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.
Change 530786 merged by jenkins-bot:
[mediawiki/core@master] Clean up ApiEditPage content handler undo logic
Change 530869 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Revert "Clean up ApiEditPage content handler undo logic"
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"
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