WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 months ago

#15729 accepted defect (bug)

Better UX after supplying incorrect information to setup-config

Reported by: nacin Owned by: kapeels
Milestone: Future Release Priority: low
Severity: normal Version:
Component: Upgrade/Install Keywords: gci has-patch 3.2-early
Focuses: Cc:

Description

In #15682 I added a 'Try Again' button to step 2 of setup-config.php if the credentials were incorrect.

The button is a simple link with a javascript:history.go(-1) hack. This should instead be a form with hidden values that submits back to setup-config.php?step=1, and fills out the form so they can try again.

Additional enhancement: If the prefix is malformed (can only contain letters, numbers and underscores), we do a wp_die(). We should again have a form with a 'Try Again' button that returns them.

As an added bonus, incorrect credentials should be filled out (for reference) but marked as incorrect and potentially with the focus set on that field.

Only caveat I can think of: This will require the use of esc_attr(), which we don't have access to yet. Not sure how to solve that without reverting to htmlspecialchars() with some extra work.

Attachments (6)

15729-2.diff (6.5 KB) - added by kapeels 3 years ago.
15729-3.diff (6.5 KB) - added by kapeels 3 years ago.
Forgot not to use heredoc and nowdoc. This one doesn't.
15729-4.diff (7.3 KB) - added by kapeels 3 years ago.
15729-6.diff (7.5 KB) - added by kapeels 3 years ago.
15729-7.diff (7.8 KB) - added by kapeels 3 years ago.
15729-8.diff (7.8 KB) - added by kapeels 3 years ago.
This has got to be commitable now :D

Download all attachments as: .zip

Change History (16)

comment:1 kapeels3 years ago

  • Cc kapeel.sable@… added
  • Owner set to kapeels
  • Status changed from new to accepted

comment:2 kapeels3 years ago

I'm on it for GCI.

comment:3 in reply to: ↑ description kapeels3 years ago

Replying to nacin:

As an added bonus, incorrect credentials should be filled out (for reference) but marked as incorrect and potentially with the focus set on that field.

Do you mean that the fields have been left incomplete? I can't find a way to determine which field has been wrongly filled.

Focus can be set, but, how to "mark" them as incorrect? By changing border colors to red?

comment:4 nacin3 years ago

You're right, it's tough. There's no way to tell if it's user, server, or password. But there is a differentiation between these three:

  • user, host, password
  • database name
  • table prefix

In lieu of border colors, let's just try a simple error message at the top in <p> tags, with three different error messages (one for each case). "Cannot check to the database server with this username and password.", "Cannot select this database.", "The table prefix can contain only letters, numbers, and underscores."

Then, focus the first applicable field via injected JS, either name, user, or table prefix.

kapeels3 years ago

kapeels3 years ago

Forgot not to use heredoc and nowdoc. This one doesn't.

comment:5 kapeels3 years ago

  • Cc kapeel.sable@… removed
  • Keywords has-patch 2nd-opinion added; needs-patch removed

comment:6 nacin3 years ago

  • Keywords 2nd-opinion removed

Minor thing, but watch for trailing whitespace.

Take a look at our coding standards: http://codex.wordpress.org/WordPress_Coding_Standards. Function names should be written with underscores separating the words.
I think we can avoid a new function all together. That would also avoid the overuse of globals here. What we can do is create our own WP_Error object, then modify the branching a bit to allow the DB connection to only be tested if the prefix is valid, and either way, we can then show a form to send them back to. It doesn't even need to be a WP_Error object
, just something we can keep track of.

For both hidden elements and in the form inputs, we need to properly escape these. We don't have access to esc_attr() here, so a call to htmlspecialchars( $string, ENT_QUOTES ) sh
ould do. Rather than just doing this for the prefix. Also, always escape as late as possible -- so in the form in both cases.

I think we're close. Nice work so far.

kapeels3 years ago

kapeels3 years ago

comment:7 nacin3 years ago

  • Keywords 3.2-early added

Last patch looks great. :-)

Final feedback:

  • Not sure why display_header() was un-indented. It should be one indent inside of the case.
  • Rename the 'passwrd' varto 'password' or 'pwd'. It looks like a typo.
  • Initialize setup_error as a new WP_Error. Use ->add() inside the preg_match. Remove the setup_error == null check and make that into an else for the preg_match.

kapeels3 years ago

kapeels3 years ago

This has got to be commitable now :D

comment:8 SergeyBiryukov3 years ago

Perhaps we should add the charset parameter to all instances of htmlspecialchars(). Though I'm not exactly sure if MySQL allows UTF-8 characters in database name, username or password, I've tried to create a MySQL user with Cyrillic username and it seems to be working.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

comment:9 SergeyBiryukov8 months ago

The button is a simple link with a javascript:history.go(-1) hack. This should instead be a form with hidden values that submits back to setup-config.php?step=1, and fills out the form so they can try again.

This would resolve #24836.

Additional enhancement: If the prefix is malformed (can only contain letters, numbers and underscores), we do a wp_die(). We should again have a form with a 'Try Again' button that returns them.

Related: #19970

Note: See TracTickets for help on using tickets.