Make WordPress Core

Opened 14 years ago

Last modified 5 years ago

#15001 new defect (bug)

Duplication and incompatibilities in register_new_user() and wp_insert_user()

Reported by: coffee2code's profile coffee2code Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Users Keywords: has-patch needs-testing needs-refresh needs-unit-tests
Focuses: Cc:

Description

As a result of [12778], the commit of a patch that was part of #11644 (the MU-merge ticket), wp_insert_user() was modified to introduce user verification checks. The addition of these checks also introduced a number of duplications and incompatibilities between it and register_new_user(). (Bear in mind that register_new_user() calls wp_create_user() which calls wp_insert_user().)

These issues include:

  • Duplication (both run-time execution and code): both functions perform username_exists() and email_exists() checks. Ideally, we should only perform each check once, and from a single piece of code.
  • Whereas both functions perform username_exists() and email_exists() checks, register_new_user() performs more checks (empty_username, invalid_username, empty_email, invalid_email). If the former 2 are being checked, all 6 criteria should be checked.
  • wp_insert_user() can now return a WP_Error object, but register_new_user() can't handle it (I reported this in #14290)
  • register_new_user() generates a new generic error if wp_create_user() (via wp_insert_user()) returns an error (assuming proper patch #14290), rather than passing along the more specific error it was told about

If an error is returned by wp_create_user(), register_new_user() throws a generic 'registerfail' error ('<strong>ERROR</strong>: Couldn&#8217;t register you... please contact the <a href="mailto:%s">webmaster</a> !'). However, most likely it received a more informative WP_Error object.

  • register_new_user() allows errors to be suppressed via filters, but wp_insert_user() does not

register_new_user() has the filter 'registration_errors' which allows plugins to suppress any encountered errors. Having done so, register_new_users()'s subsequent call to wp_create_user() can generate an error (of the type already suppressed) which is then un-suppressable. This renders the 'registration_errors' filter useless for the username_exists and email_exists errors (and possibly other in the future).


These different issues may warrant separate tickets, but cumulatively I think they point to the need to refactor register_new_user() and wp_insert_user() to be more efficient and compatible.

This is a rare instance where I don't include an immediate patch for a ticket, but I wanted to get it out there and see if we can try and get these fixed for 3.1.

Attachments (2)

combine-user-error-checking.diff (6.3 KB) - added by wonderboymusic 12 years ago.
15001.diff (6.4 KB) - added by wonderboymusic 11 years ago.

Download all attachments as: .zip

Change History (10)

#1 @hakre
14 years ago

Your considerations look very useful to me. Maybe starting with removing code duplication and putting the shared code into functions?

#2 @mdawaffe
14 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#3 @wonderboymusic
12 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Combined code from both functions - passes unit tests / needs more testing. Patch attached.

#4 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

Refreshed against trunk, I think this is still a good move - the code between the two funcs should be in sync

#5 @nacin
11 years ago

  • Keywords needs-refresh added

#14290 means this will need a refresh and possible reconsideration.

#6 @wonderboymusic
11 years ago

  • Milestone changed from 3.7 to 3.8

#7 @wonderboymusic
11 years ago

  • Milestone changed from 3.8 to Future Release

#8 @chriscct7
9 years ago

  • Keywords needs-unit-tests added
  • Version changed from 3.0.1 to 3.0
Note: See TracTickets for help on using tickets.