Change #1102408 had a related patch set uploaded (by Dmaza; author: Dmaza):

[mediawiki/core@master] [WIP]

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

I changed the acceptance criteria to use the existing id parameter, instead of a new blockid. https://en.wikipedia.org/w/api.php?action=help&modules=unblock

Change it back if that doesn't work, for whatever reason.

I don't understand the rationale for removing multiple blocks with a single post to action=unblock. Things would be simpler if we just didn't do that.

What is the scenario in which removing multiple blocks would be the right thing to do?

Each subsidiary unblock is a logged write action. If a user is blocked 10 times, 10 unblock log entries will appear when this request is posted. If a gadget like Twinkle gives us ambiguous parameters, isn't it safer to return an error message rather than to delete an arbitrary number of blocks?

In the WIP patch, the result of a successful unblock is an array of blocks, instead of a single block. That's a b/c break. Giving an error message is not a b/c break.

The WIP patch also breaks b/c in the middle layer, with an array in the Status returned by UnblockUser::unblock() instead of a single block. In the implementation, everything it does now needs to be wrapped in a loop. What is it for? Who needs that?

In T378148#10431220, @tstarling wrote:

I don't understand the rationale for removing multiple blocks with a single post to action=unblock. Things would be simpler if we just didn't do that.

I agree. The rationale is that an admin might want to unblock an account entirely by removing all blocks (even if rarely). This is a convenience feature.

What is the scenario in which removing multiple blocks would be the right thing to do?

Same as above. It is a rather unlikely scenario but so it is to have 50 concurrent blocks on an account, which is also supported.

Each subsidiary unblock is a logged write action. If a user is blocked 10 times, 10 unblock log entries will appear when this request is posted. If a gadget like Twinkle gives us ambiguous parameters, isn't it safer to return an error message rather than to delete an arbitrary number of blocks?

It is not ambiguous in my opinion to pass the user to ?action=unblock

In the WIP patch, the result of a successful unblock is an array of blocks, instead of a single block. That's a b/c break. Giving an error message is not a b/c break.

@MusikAnimal looked around and didn't find any usage. We discussed this with @Cparle and decided that a b/c break was ok in this case as long as we send an email to mediawiki-api-announce about the change

Happy to wrap up my WIP with your suggestion if the b/c is an issue or if we think that, due to its rarity, it doesn’t warrant the need for the feature. cc @JWheeler-WMF

I'm worried less about b/c and more about adding code and complexity for no reason.

My point about b/c is that b/c does not provide a rationale for this change. This is not the more compatible path.

If an admin wants to unblock an account entirely, removing all blocks, they are going to need a UI, not an API. The admin will need to see a list of existing blocks to understand what they are doing. The UI can then do multiple API requests. But if I understand correctly, we're not planning to add such a UI so there is no need to add support for it right now.

We discussed this with @Cparle and decided that a b/c break was ok in this case as long as we send an email to mediawiki-api-announce about the change

Yes, a b/c break is OK as long as you send an email. But I'm not going to approve this change unless there is a good reason for making it.

We discussed this with @Cparle and decided that a b/c break was ok in this case as long as we send an email to mediawiki-api-announce about the change

Yes, a b/c break is OK as long as you send an email. But I'm not going to approve this change unless there is a good reason for making it.

Clearly a conversation that needs to happen with @Cparle and @JWheeler-WMF so we all can get in the same page. I'm happy to do it either way.

In T378148#10435788, @tstarling wrote:

If an admin wants to unblock an account entirely, removing all blocks, they are going to need a UI, not an API. The admin will need to see a list of existing blocks to understand what they are doing. The UI can then do multiple API requests. But if I understand correctly, we're not planning to add such a UI so there is no need to add support for it right now.

I believe we did want a "Remove all blocks" button at Special:Block, but it is merely a nice-to-have. What you're saying makes sense to me; Maybe it's cleaner for the client (Special:Block in this case) to remove each individual block, not the API. The fact that it would produce N log entries for N block removals is what makes me think the API shouldn't be responsible for that. Are there any other action API endpoints that allow you to make multiple log entries, barring use of a generator? If not, I say we don't need to set that precedent.

In T378148#10436077, @MusikAnimal wrote:

Are there any other action API endpoints that allow you to make multiple log entries, barring use of a generator? If not, I say we don't need to set that precedent.

Interesting question. I think there are three in core. One kind of OK, one fairly harmless, and one so alarming that I might have to file a security vulnerability for it. It's certainly unconventional to allow multiple logged actions. Commonly-used actions like edit, delete, move and rollback only do one thing.

Right now the API accepts a user, and the user's blocks (of which there can be only one atm) are removed. That was the point of the third bullet point in the ticket - to maintain this functionality to avoid b/c breaks.

In T378148#10431220, @tstarling wrote:

In the WIP patch, the result of a successful unblock is an array of blocks, instead of a single block. That's a b/c break. Giving an error message is not a b/c break.

A change in the result format is obvs a b/c break, but surely a change in the meaning of the user param is also a b/c break? If sending user no longer deletes the block(s) for that user, what meaning does the param have?

In T378148#10436654, @Cparle wrote:

Right now the API accepts a user, and the user's blocks (of which there can be only one atm) are removed. That was the point of the third bullet point in the ticket - to maintain this functionality to avoid b/c breaks.

The third bullet point breaks b/c, adds complexity, adds a feature nobody is asking for, and interprets user input in a potentially harmful and unexpected way.

A change in the result format is obvs a b/c break, but surely a change in the meaning of the user param is also a b/c break? If sending user no longer deletes the block(s) for that user, what meaning does the param have?

Giving an error in the multiblocks case doesn't change the interface. Clients already know what errors are.

The meaning of the user parameter is as follows: if it is given and there is one block, delete the block. If there are two blocks, give an error.

How does this look from the perspective of the person using this? The motivating use case for multiblocks was like this: suppose the Arbcom imposes a topic ban on a user who we will call Jason. For 18 months, Jason is no longer allowed to edit articles related to spoons. Arbcom member Isaac creates a partial block with an expiry time of 18 months blocking Jason from editing various spoon-related articles. Now Jason insults admin Kate on her talk page, and so Kate adds an indefinite sitewide ban on Jason with the reason field set to something like "get lost Jason". Leah, watching this exchange via an Emacs extension which listens to our IRC feed, decides to reverse Kate's block, and so she uses the unblock-user macro she added to Emacs some years previously.

Scenario one: Two log entries appear: Leah removed the block by Kate, and Leah removed the block by Isaac. Maybe Isaac notices and Leah is forced to explain the whole mess on a lengthy WP:AN/I thread which links to this task. Or maybe nobody notices and Jason starts trolling the spoon pages again.

Scenario two: Leah gets an error message "Two blocks for this user exist with IDs 3334 and 3433. Please delete these blocks individually." Maybe Leah updates her code. Or maybe she uses the website like a normal person.

In T378148#10436812, @tstarling wrote:

Giving an error in the multiblocks case doesn't change the interface. Clients already know what errors are.

A call that used to not return an error suddenly returning an error is changing the interface, surely? However ...

In T378148#10436812, @tstarling wrote:

suppose the Arbcom imposes a topic ban on a user who we will call Jason. For 18 months, Jason is no longer allowed to edit articles related to spoons ...

Scenario one: Two log entries appear: Leah removed the block by Kate, and Leah removed the block by Isaac. Maybe Isaac notices and Leah is forced to explain the whole mess on a lengthy WP:AN/I thread which links to this task. Or maybe nobody notices and Jason starts trolling the spoon pages again.

Scenario two: Leah gets an error message "Two blocks for this user exist with IDs 3334 and 3433. Please delete these blocks individually." Maybe Leah updates her code. Or maybe she uses the website like a normal person.

Heh ok fair enough this makes total sense. I concede defeat :p

Updated the ticket description

In T378148#10436137, @tstarling wrote:

One kind of OK, one fairly harmless, and one so alarming that I might have to file a security vulnerability for it.

I filed T383196.

Change #1102408 merged by jenkins-bot:

[mediawiki/core@master] ApiUnblock: Add support for multiblocks

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