Page MenuHomePhabricator

Special:UserLogin calls Status::newFatal incorrectly on account creation, generates odd API error codes
Closed, ResolvedPublic

Description

Gerrit change 17952 changed Special:UserLogin to return a Status object from the account creation function, passing it a RawMessage which breaks the expectation of the API's dieStatus method that it can get a sane key from the message object's getKey method.

But then Gerrit change 47821 broke things worse by changing Special:UserLogin to pass a raw string rather than a Message object or message key as Status::newFatal expects.


Version: 1.23.0
Severity: normal

Details

Reference
bz60008

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 2:51 AM
bzimport set Reference to bz60008.
Anomie created this task.Jan 13 2014, 5:42 PM
brion added a comment.Jan 17 2014, 7:45 PM

Taking this as we want this to work smoothly for mobile apps.

brion added a comment.Jan 17 2014, 8:00 PM

I think the thing to do here is to allow sending back a *useful* Status object from the hook in place of just a string, which should be able to return a sane error. Lemme try...

The issue here is that the Status class is too intertwined with the Message class, and this was even before RawMessage was introduced. When you made a Status object, it was assumed any error keys were backed by messages.

The solution should be that the concerns need to be separated. The Status class should not have anything to do with the Message class. However, that may be a bit difficult to achieve, so at the very least there needs to be a separation between error codes and message keys.

Change 108088 had a related patch set uploaded by Brion VIBBER:
Add Status outparam for AbortNewAccount hook to fix API error handling

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

Change 108089 had a related patch set uploaded by Brion VIBBER:
Update ConfirmEdit to return Status object on AbortNewAccount hook

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

brion added a comment.Jan 17 2014, 8:59 PM

(In reply to comment #3)

The issue here is that the Status class is too intertwined with the Message
class, and this was even before RawMessage was introduced. When you made a
Status object, it was assumed any error keys were backed by messages.
The solution should be that the concerns need to be separated. The Status
class
should not have anything to do with the Message class. However, that may be a
bit difficult to achieve, so at the very least there needs to be a separation
between error codes and message keys.

I'd rather not redo the entire interface of SpecialUserlogin and ApiCreateAccount just to get this going. Any strong objections to using this existing approach as a basic fix?

(In reply to comment #6)

Any strong objections to using this existing approach as a basic fix?

Nope (as indicated by my +1).

Change 108089 merged by jenkins-bot:
Update ConfirmEdit to return Status object on AbortNewAccount hook

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

Change 108088 merged by jenkins-bot:
Add Status outparam for AbortNewAccount hook to fix API error handling

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

Change 111113 had a related patch set uploaded by JGonera:
Add Status outparam for AbortNewAccount hook to fix API error handling

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

Change 111118 had a related patch set uploaded by MaxSem:
Update ConfirmEdit to return Status object on AbortNewAccount hook

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

Change 111118 merged by jenkins-bot:
Update ConfirmEdit to return Status object on AbortNewAccount hook

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

Change 111113 merged by MaxSem:
Add Status outparam for AbortNewAccount hook to fix API error handling

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