Make WordPress Core

Opened 5 years ago

Last modified 5 years ago

#47954 new enhancement

Create a sanity check for siteurl and home changes

Reported by: otto42's profile Otto42 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: ui, administration Cc:

Description (last modified by Otto42)

One of the most common problems is when people change the site URL fields on the settings general screen, without full understanding of what it means. It's a common enough problem that we've had documentation on ways to fix it for ages:

https://wordpress.org/support/article/changing-the-site-url/

The idea is to enhance this field so as to perform some kind of sanity check before actually changing the fields. Health check exists, we have checks for plugin and theme changes to some degree. This could be checked as well.

Concept: Change the field, make an internal request, see if it fails, if so change them back, then warn the user with a "hey, this change seems like it's gonna break things real bad, are you really sure?" Or equivalent.

Second thought: when these change, email the admin user about them, with a revert link that will work no matter what.

I'm sure that other ideas exist as well.

Attachments (2)

47954.diff (16.8 KB) - added by donmhico 5 years ago.
Email admin a restore link to revert siteurl and home option changes.
47954.2.diff (17.0 KB) - added by donmhico 5 years ago.
Make the expiry time filterable.

Download all attachments as: .zip

Change History (14)

#2 @Otto42
5 years ago

  • Description modified (diff)

#3 @donmhico
5 years ago

This is interesting. IMHO we can do both of the approach @Otto42 mentioned above.

Change the field, make an internal request, see if it fails, if so change them back, then warn the user with a "hey, this change seems like it's gonna break things real bad, are you really sure?" Or equivalent.

At the top of my head, maybe we can try to use the constants WP_HOME and WP_SITEURL when doing the internal request. The request can be simple, maybe just see if both front-end and wp-admin both sends back 200 HTTP?

I'll try to create an initial patch when time permits.

#4 @donmhico
5 years ago

Here's an initial code that should perform the second approach @Otto42 suggested.

Second thought: when these change, email the admin user about them, with a revert link that will work no matter what.

Feel free to try this code by adding in your theme's functions.php. It still needs unit testing and putting the code to a much suited place. I'll try to put it as a patch later.

<?php
/**
  * Setup restore plan to give the admin a way to revert the 'siteurl' and/or 'home' changes. 
  *
  * This function performs the following tasks.
  * 1. Creates a 'siteurl_restore_key' transient.
  * 2. Save the old 'siteurl' and 'home' options as transients.
  * 3. Email the admin the regarding the change and give a restore link.
  *
  * Known Issue / Can be enhanced.
  * 1. The email action part is firing twice if both 'siteurl' and 'home' is updated in 
  *    'wp-admin/options-general.php' at the same time.
  * 2. Provide a filter for the email subject and message.
  * 
  * @since 5.3.0
  *
  * @param string $option
  * @param string $old_value
  * @param string $value
  */
function setup_siteurl_restore( $option, $old_value, $value ) {
        if ( 'siteurl' === $option || 'home' === $option ) {
                $restore_key = wp_generate_password( 12, false );
                
                set_transient( 'siteurl_restore_key', $restore_key, 1800 );
                set_transient( "old_{$option}", $old_value, 1800 );

                $restore_url = add_query_arg( [
                        'srk' => $restore_key,
                ], $old_value );

                wp_mail( get_option('admin_email'), 'Your WordPress site url was changed.', 
                        "You can undo this change by clicking this link {$restore_url}" );
        }
}
add_action( 'updated_option', 'setup_siteurl_restore', 10, 3 );

/**
 * Perform the siteurl restore operation.
 *
 * Note: I only hook this function in 'registered_taxonomy' because I think the sooner this
 * function is fired, the better. 
 * 
 * @since 5.3.0
 */
function perform_siteurl_restore() {
        if ( isset( $_GET['srk'] ) && ! empty( $_GET['srk'] ) ) {
                $restore_key = get_transient( 'siteurl_restore_key' );

                if ( $_GET['srk'] === $restore_key ) {
                        $old_siteurl = get_transient( 'old_siteurl' );
                        $old_home        = get_transient( 'old_home' );

                        $is_restore_success = false;

                        if ( $old_siteurl ) {
                                $update_siteurl = update_option( 'siteurl', $old_siteurl );

                                if ( $update_siteurl ) {
                                        delete_transient( 'old_siteurl' );

                                        $is_restore_success = true;
                                }
                        }

                        if ( $old_home ) {
                                $update_home = update_option( 'home', $old_home );

                                if ( $update_home ) {
                                        delete_transient( 'old_home' );

                                        $is_restore_success = true;
                                }
                        }

                        if ( $is_restore_success ) {
                                delete_transient( 'siteurl_restore_key' );

                                // Track success.
                                set_transient( 'siteurl_restore_success', '1', 300 );

                                $restored_site_admin_url = trailingslashit( get_option( 'home' ) ) . 'wp-admin';

                                // Success redirect url.
                                $success_redirect_url = add_query_arg( 'srsuccess', '1', $restored_site_admin_url );

                                wp_redirect( $success_redirect_url );
                                exit;
                        }
                }
                else {
                        wp_die( __( 'Restore key is invalid.' ) );
                        exit;
                }
        }
}
add_action( 'registered_taxonomy', 'perform_siteurl_restore' );

/**
 * Display success notice if the restore siteurl operation is a success.
 *
 * @since 5.3.0
 */
function restore_siteurl_success_notice() {
        if ( isset( $_GET['srsuccess'] ) && '1' === $_GET['srsuccess'] ) {
                $restore_success = get_transient( 'siteurl_restore_success' );

                if ( '1' === $restore_success ) {
                        add_action( 'admin_notices', 'restore_siteurl_success_notice__success');

                        delete_transient( 'siteurl_restore_success' );
                }
        }
}
add_action( 'admin_init', 'restore_siteurl_success_notice' );

/**
 * Restore siteurl success notice.
 *
 * @since 5.3.0
 */
function restore_siteurl_success_notice__success() {
        $class = 'notice notice-success';
        $message = __( 'Your WordPress site url was successfully restored.' );

        printf( '<div class="%1$s"><p>%2$s</p></div>', esc_attr( $class ), esc_html( $message ) ); 
}
Last edited 5 years ago by donmhico (previous) (diff)

#5 @donmhico
5 years ago

Upon looking further, I just found out that since 5.2.0, WP_Recovery_Mode class was introduced. Correct me if i'm wrong, but from what I see, it's use as a way for site admins to enter the website if ever a plugin / theme fails. Do you think this Change Siteurl recovery should also be part of WP_Recovery_Mode?

I am asking this because the header in WP_Recovery_Mode class file states

<?php
/**
 * Error Protection API: WP_Recovery_Mode class
 *
 * @package WordPress
 * @since   5.2.0
 */

And this new Change Siteurl recover feature can be counted as an Error Protection function.

Any thoughts?

@donmhico
5 years ago

Email admin a restore link to revert siteurl and home option changes.

#6 @donmhico
5 years ago

  • Keywords has-patch added

The attached patch, 47954.diff, should does the second approach @Otto42 mentioned.

Second thought: when these change, email the admin user about them, with a revert link that will work no matter what.

Please feel free to test it so we can further enhance it or fix any bugs on it.

As for

Concept: Change the field, make an internal request, see if it fails, if so change them back, then warn the user with a "hey, this change seems like it's gonna break things real bad, are you really sure?" Or equivalent.

I'll be working on this when time permits.

I also created a fork branch for this in Github. Again, feel free to help out.
https://github.com/donmhico/wordpress-develop/tree/trac-47954

@Otto42 @SergeyBiryukov - I'm not sure if this should be early for the next release but it would be awesome if we can add this to 5.3 milestone even just this email to admin a restore link that it currently has as IMHO, it would really help site admins, especially non-techie ones. Thanks.

This ticket was mentioned in Slack in #core by donmhico. View the logs.


5 years ago

#8 @Clorith
5 years ago

Had a quick look, and the concept isn't bad.

I would prefer if it used core constants for the expiry-time, instead of plain numbers, it makes it easier to discern what the time actually is, and would probably also do well with a filter to modify the expiry time. The default time of 30 minutes sounds reasonable though.

Using 30 * MINUTE_IN_SECONDS would make this default much clearer.

I noticed the restore feature is hooked to registered_taxonomy, this needs a much earlier hook, a quick test with init shows that even that is too late. WordPress does an automatic redirect to the URL configured, and that redirect is a 301, which means browsers will cache the redirect, making it near impossible for the average user to utilize the restore URL.

--

For the part that's not done yet, which I personally think is the more important aspect of this, the loopback to check if a new URL will still work before the user is locked out, this should be one function for all of core (I've made mention of this elsewhere as well today), and I would prefer to see a proper implementation of such a function for use here first.

@donmhico
5 years ago

Make the expiry time filterable.

#9 @donmhico
5 years ago

Thanks for the feedback @Clorith. The latest patch, 47954.2.diff, applies the changes you recommended. The expiry time is now using 30 * MINUTE_IN_SECONDS and also filterable. See https://github.com/donmhico/wordpress-develop/commit/19617ea6d50573919d4b775072fcb8e8a170e3e8

Regarding where it is hooked. Correct me if i'm wrong, but registered_taxonomy is the earliest possible action I can hook it to where the pluggable.php is already loaded. The restore function uses wp_redirect() which is declared in pluggable.php, hence if I want to hook it in muplugins_loaded, I need to either load pluggable.php when needed to or not use wp_redirect(). Unless there's any reason why it should be hooked in muplugins_loaded, then it's good to hook it when pluggable.php is already loaded.

#10 @donmhico
5 years ago

Concept: Change the field, make an internal request, see if it fails, if so change them back, then warn the user with a "hey, this change seems like it's gonna break things real bad, are you really sure?" Or equivalent.

What would be a sane way to check? Looking at https://wordpress.org/support/article/moving-wordpress/, it mentions on some steps that 404 is expected since the instructions prioritize the Change of urls before moving the files / database. In this case, it's hard to think of a sanity check.

Of course, it would be different if we are just trying is to prevent 'accidental' URL change in which case, we can just check if home or siteurl is about to be updated and just ask for a "Are you sure?" confirmation to the admin.

#11 @SergeyBiryukov
5 years ago

#48165 was marked as a duplicate.

#12 @felipeloureirosantos
5 years ago

I agree that it is a point that would really improve the user experience. Also, maybe we could consider just taking this fields out.

We could take out the option to users edit it on wp-admin/options-general.php, so they just would be able to do it on the hidden wp-admin/options.php inside the admin panel. Maybe we could still show it there, but do not allow the change.

Note: See TracTickets for help on using tickets.