Last modified: 2014-02-04 17:24:58 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T62008, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 60008 - Special:UserLogin calls Status::newFatal incorrectly on account creation, generates odd API error codes
Special:UserLogin calls Status::newFatal incorrectly on account creation, gen...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
1.23.0
All All
: Normal normal (vote)
: ---
Assigned To: Brion Vibber
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-13 17:42 UTC by Brad Jorsch
Modified: 2014-02-04 17:24 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Brad Jorsch 2014-01-13 17:42:26 UTC
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.
Comment 1 Brion Vibber 2014-01-17 19:45:46 UTC
Taking this as we want this to work smoothly for mobile apps.
Comment 2 Brion Vibber 2014-01-17 20:00:05 UTC
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...
Comment 3 Tyler Romeo 2014-01-17 20:29:05 UTC
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.
Comment 4 Gerrit Notification Bot 2014-01-17 20:33:28 UTC
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
Comment 5 Gerrit Notification Bot 2014-01-17 20:36:17 UTC
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
Comment 6 Brion Vibber 2014-01-17 20:59:35 UTC
(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?
Comment 7 Tyler Romeo 2014-01-17 21:06:53 UTC
(In reply to comment #6)
> Any strong objections to using this existing approach as a basic fix?

Nope (as indicated by my +1).
Comment 8 Brion Vibber 2014-01-17 21:37:24 UTC
Yay! :)
Comment 9 Gerrit Notification Bot 2014-01-17 23:32:53 UTC
Change 108089 merged by jenkins-bot:
Update ConfirmEdit to return Status object on AbortNewAccount hook

https://gerrit.wikimedia.org/r/108089
Comment 10 Gerrit Notification Bot 2014-01-23 20:27:02 UTC
Change 108088 merged by jenkins-bot:
Add Status outparam for AbortNewAccount hook to fix API error handling

https://gerrit.wikimedia.org/r/108088
Comment 11 Gerrit Notification Bot 2014-02-03 23:10:16 UTC
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
Comment 12 Gerrit Notification Bot 2014-02-03 23:23:42 UTC
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
Comment 13 Gerrit Notification Bot 2014-02-03 23:24:38 UTC
Change 111118 merged by jenkins-bot:
Update ConfirmEdit to return Status object on AbortNewAccount hook

https://gerrit.wikimedia.org/r/111118
Comment 14 Gerrit Notification Bot 2014-02-03 23:26:45 UTC
Change 111113 merged by MaxSem:
Add Status outparam for AbortNewAccount hook to fix API error handling

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

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links