WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#9569 closed enhancement (fixed)

Installing over existing user table gives misleading instruction

Reported by: coffee2code Owned by:
Milestone: 2.8 Priority: lowest
Severity: trivial Version: 2.8
Component: Upgrade/Install Keywords: has-patch tested commit
Focuses: Cc:

Description

WordPress properly handles being installed into a database with an existing wp_users table (and potentially no other tables). In this situation, instead of generating a random password, WP reports:

User already exists. Password inherited.

However, just below that, it also reports:

Note that password carefully! It is a random password that was generated just for you.

The latter statement is incorrect in this scenario, and could be misleading (and concerning) to the person doing the installation. Nothing is in jeopardy, though.

Granted, this is a bit of an edge case. But the fix is trivial (it compares the password against the first message listed above). If the message is later changed in one place and not the other, the code will just fall back to the currently existing behavior.

Attachments (2)

bug9569.inherited-password-is-not-random.diff (725 bytes) - added by coffee2code 5 years ago.
Aforementioned patch
9569.2.diff (697 bytes) - added by coffee2code 5 years ago.
Alternative (and preferred) implementation

Download all attachments as: .zip

Change History (10)

coffee2code5 years ago

Aforementioned patch

comment:1 Denis-de-Bernardy5 years ago

  • Keywords tested added

patch is better than none, but we should keep this one open even after the commit, due to the fact that if the function gets overridden then the password may not necessarily contain that string.

comment:2 coffee2code5 years ago

Attaching an alternative implementation of the patch. Just about as simple, but more generic, and probably good enough to warrant closing the ticket if committed. No longer compare the returned random password with a separate copy of the phrase indicating an existing password was inherited, which is a potentially brittle check as the phrase could be changed in one place but not the other.

Instead, simply check the potentially generated password for a space character. A space character is not a valid password character. Its presence would indicate a phrase or sentence of some sort was returned, therefore the instruction to note the password need not be shown. This allows for the message to be changed without needing to change anything here. Also, some other error message could just as well be returned and things will still behave as expected.

coffee2code5 years ago

Alternative (and preferred) implementation

comment:3 Denis-de-Bernardy5 years ago

this:

strpos($password, ' ') === false

will not work on a chinese install. :-)

comment:4 follow-up: coffee2code5 years ago

Ah, oops. Any other idea for something simple? Otherwise, perhaps just the original patch for the time being? Inline comments could be added to both places the message is defined to tell anyone updating the text that it needs to be updated in the other place as well.

Other possible considerations:

  • Have wp_install() return '' instead of the "...Password inherited..." message as the password. The presumption being that no password == inherited password. Therefore, the "...Password inherited..." message could just be specified in one place (wp-admin/install.php) and not also in wp_install().
  • Or if we anticipate later having other non-password responses such that '' can't be assumed to mean a password was inherited, maybe return a WP_Error object in place of the password and check for that?

comment:5 in reply to: ↑ 4 Denis-de-Bernardy5 years ago

Replying to coffee2code:

  • Have wp_install() return '' instead of the "...Password inherited..." message as the password. The presumption being that no password == inherited password. Therefore, the "...Password inherited..." message could just be specified in one place (wp-admin/install.php) and not also in wp_install().
  • Or if we anticipate later having other non-password responses such that '' can't be assumed to mean a password was inherited, maybe return a WP_Error object in place of the password and check for that?

WP_Error object seems like the one that makes the most sense. That it's called WP_Error might confuse whoever looks into it after, though. A WP_Warning comes to mind. :-D

D.

comment:6 Denis-de-Bernardy5 years ago

  • Keywords commit added

comment:7 westi5 years ago

I think the best thing here is to move the string generation into wp_install and return the message for both cases from there.

comment:8 westi5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [11326]) Give better instructions when installing over an existing users table. Fixes #9569 based on patch from coffee2code.

Note: See TracTickets for help on using tickets.