Last modified: 2014-03-13 21:13:16 UTC
Should check for blocks, existing usernames, and other issues in the first request, and then ask for a captcha. Right now, if the user tries to register a name already registered, we ask them to solve a captcha and *then* tell them it is taken. And then they have to try again, and we ask them for a captcha solving before proceeding. This is terrible UX, and I can't think of a reason why this has to be the case.
If you want to refactor LoginForm::addNewaccountInternal() into a validation function and an actual creation function, feel free and I'll review it. Keep in mind that we'll most likely still want to run 'AddNewAccountApiForm' before 'AbortNewAccount'. But let's not duplicate all the checks in LoginForm::addNewaccountInternal() into ApiCreateAccount.
Also see patch in bug 34447
Change 116872 had a related patch set uploaded by MaxSem: action=createaccount should do checks in the first request, not second https://gerrit.wikimedia.org/r/116872
Looking at this closer, I think the description of this bug is misleading. Which means comment 1 should be ignored, at least as far as ConfirmEdit goes. The problem isn't the order of the checks, it's that the captcha information is added at the "NeedToken" step (implying it needs to be solved there) and not in a later step. That should probably be the other way around. So what should really be happening is that ConfirmEdit shouldn't use the 'AddNewAccountApiResult' hook at all. Instead, the case in ApiCreateAccount handling a not-OK status should call some other hook function to populate an $extraData array (which ConfirmEdit would use to inject its captcha data), then call $this->getErrorFromStatus() and $this->dieUsage($msg, $code, 0, $extraData). Or we could add an $extraData parameter to ApiBase::dieStatus. And note you're probably still going to run into problems of this sort if there are multiple AbortNewAccount hook functions, since ConfirmEdit's might be run before the other extensions'. There's nothing the API can do about that, it would take redefining AbortNewAccount to still fail the creation when $abortStatus is non-OK even if the hook function returned true (and then probably deprecate returning non-true for that hook).
Change 116872 abandoned by MaxSem: action=createaccount should do checks in the first request, not second https://gerrit.wikimedia.org/r/116872
After poking at it some more, what would fully help is to perform all checks but token, then call AbortNewAccount, then check token and only then call some new hook for ConfirmEdit ("AbortNewAccountForRealz"?). Simply adding extra data to dieStatus() would result in clients having to handle two cases with principally diffrent data structure: token is needed (normal return with {createaccount:{result:"NeedsToken", captcha:...}}) and all other problems ({error:{...}}).
Change 118374 had a related patch set uploaded by MaxSem: Allow extensions modify API action=createaccount errors https://gerrit.wikimedia.org/r/118374
Change 118381 had a related patch set uploaded by MaxSem: Return captcha information via createaccount API only if no other errors https://gerrit.wikimedia.org/r/118381
Change 118374 abandoned by MaxSem: Allow extensions modify API action=createaccount errors Reason: Ehm, not needed. https://gerrit.wikimedia.org/r/118374
Change 118381 merged by Brion VIBBER: Return captcha information via createaccount API only if no other errors https://gerrit.wikimedia.org/r/118381
Ok, 8a336ddd9 from that last change set tweaks things so the initial 'NeedsToken' response doesn't include the captcha information. This means the user gets prompted for a captcha only after we've checked other fatal errors. Seems to work in testing. :D Will update docs. Be aware that this hasn't gone out live yet... Do we need an LD or just wait for it and make sure the apps handle the new mode?
