Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes 5667 detect domain change #5688

Merged
merged 55 commits into from
Aug 4, 2023

Conversation

CrochetFeve0251
Copy link
Contributor

@CrochetFeve0251 CrochetFeve0251 commented Jan 17, 2023

Description

This issue is related to the WP Rocket caching plugin for WordPress, where the user is requesting a feature that would display a notice after a site has been cloned to remind the user to deactivate and re-activate WP Rocket. The problem is that some users may not be aware that they need to deactivate WP Rocket before a migration or clone, and if they do not, it can lead to issues such as a non-cached or non-optimized site due to incorrect config files.
The solution proposed during grooming is to add a new subscriber class that detects a change of domain and launches the action rocket_domain_changed which will clear the cache, reset the preload cache table, and clear the config files.

Fixes #5667

Type of change

Please delete options that are not relevant.

  • Enhancement (non-breaking change which improves an existing functionality)

Is the solution different from the one proposed during the grooming?

No.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Tested locally.
  • Test with automated tests.

Checklist:

Please delete the options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@CrochetFeve0251 CrochetFeve0251 self-assigned this Jan 17, 2023
@CrochetFeve0251 CrochetFeve0251 added effort: [S] 1-2 days of estimated development time module: filesystem labels Jan 17, 2023
@CrochetFeve0251 CrochetFeve0251 requested a review from a team January 17, 2023 16:27
@CrochetFeve0251 CrochetFeve0251 marked this pull request as ready for review January 17, 2023 16:27
@CrochetFeve0251 CrochetFeve0251 changed the title Enhancement/5667 detect domain change Enhancement 5667 detect domain change Jan 18, 2023
@CrochetFeve0251 CrochetFeve0251 changed the title Enhancement 5667 detect domain change Closes 5667 detect domain change Jan 18, 2023
@vmanthos vmanthos self-requested a review January 19, 2023 06:58
@vmanthos
Copy link
Contributor

@CrochetFeve0251 I checked the PR on single (as in not multisite) sites. Both when changing www to non-www and vice versa, and when completely changing the domain name.

  • The old configuration files are deleted and new ones are correctly created.
  • The wpr_rocket_cache table is cleared and the new sitemap is loaded and preloaded.

What's not happening, and I believe it should - it will also resolve the issue mentioned by @camilamadronero-zz here), is deleting the old cache folders.

Can you please look into this? Also, take into consideration multisites. 🙏

@vmanthos
Copy link
Contributor

@CrochetFeve0251 After changing the domain name, e.g. adding or removing www, when first logging into the admin area I'm getting the following fatal error:

PHP Fatal error:  Uncaught TypeError: call_user_func_array(): Argument #1 ($callback) must be a valid callback, cannot access protected method WP_Rocket\Engine\Cache\AdminSubscriber::clear_cache() in /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php:308
Stack trace:
#0 /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters()
#1 /home/u923949912/domains/example.com/public_html/wp-includes/plugin.php(517): WP_Hook->do_action()
#2 /home/u923949912/domains/example.com/public_html/wp-content/plugins/wp-rocket/inc/Engine/Admin/DomainChange/Subscriber.php(52): do_action()
#3 /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php(308): WP_Rocket\Engine\Admin\DomainChange\Subscriber->maybe_launch_domain_changed()
#4 /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters()
#5 /home/u923949912/domains/example.com/public_html/wp-includes/plugin.php(517): WP_Hook->do_action()
#6 /home/u923949912/domains/example.com/public_html/wp-admin/admin.php(175): do_action()
#7 /home/u923949912/domains/example.com/public_html/wp-admin/plugins.php(10): require_once('/home/u92394991...')
#8 {main}
  thrown in /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php on line 308

After this, WordPress and WP Rocket is loading.

Apart from the fatal error:

  1. The cache isn't cleared.
  2. The entries in the wpr_rocket_cache table aren't removed and are not preloaded. Note that this happened when wp_rocket_last_base_url was already in the database and rocket_domain_changed fired.
  3. wp_rocket_last_base_url isn't removed when uninstalling WP Rocket.

Can you please look into these? 🙏

@CrochetFeve0251
Copy link
Contributor Author

@CrochetFeve0251 After changing the domain name, e.g. adding or removing www, when first logging into the admin area I'm getting the following fatal error:

PHP Fatal error:  Uncaught TypeError: call_user_func_array(): Argument #1 ($callback) must be a valid callback, cannot access protected method WP_Rocket\Engine\Cache\AdminSubscriber::clear_cache() in /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php:308
Stack trace:
#0 /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters()
#1 /home/u923949912/domains/example.com/public_html/wp-includes/plugin.php(517): WP_Hook->do_action()
#2 /home/u923949912/domains/example.com/public_html/wp-content/plugins/wp-rocket/inc/Engine/Admin/DomainChange/Subscriber.php(52): do_action()
#3 /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php(308): WP_Rocket\Engine\Admin\DomainChange\Subscriber->maybe_launch_domain_changed()
#4 /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters()
#5 /home/u923949912/domains/example.com/public_html/wp-includes/plugin.php(517): WP_Hook->do_action()
#6 /home/u923949912/domains/example.com/public_html/wp-admin/admin.php(175): do_action()
#7 /home/u923949912/domains/example.com/public_html/wp-admin/plugins.php(10): require_once('/home/u92394991...')
#8 {main}
  thrown in /home/u923949912/domains/example.com/public_html/wp-includes/class-wp-hook.php on line 308

After this, WordPress and WP Rocket is loading.

Apart from the fatal error:

1. The cache isn't cleared.

2. The entries in the `wpr_rocket_cache` table aren't removed and are not preloaded. Note that this happened when `wp_rocket_last_base_url` was already in the database and `rocket_domain_changed` fired.

3. `wp_rocket_last_base_url` isn't removed when uninstalling WP Rocket.

Can you please look into these? pray

@vmanthos the fatal could explain point 1. and 2.
I will fix it and look at 3.

@vmanthos
Copy link
Contributor

@CrochetFeve0251 Thank you for the fixes!

All issues have been fixed except for the cache clearing (1.).
rocket_clean_domain() runs but the URL we pass to be cleared is the current one, i.e. the one after the change.

When you have time, please check this. In the meantime, I'll continue exploratory testing.

@vmanthos
Copy link
Contributor

vmanthos commented Apr 6, 2023

@CrochetFeve0251 Additionally to my previous comment, the following part should be updated:

foreach ( $contents as $content ) {
$content = WP_ROCKET_CONFIG_PATH . $content['name'];
if ( ! preg_match( '#\.php$#', $content ) || ! $this->filesystem->is_file( $content ) || in_array(
$content,
$configs,
true
) ) {
continue;
}
$this->filesystem->delete( $content );
}

to match the logic here:

foreach ( $config_dir as $file ) {
if ( ! $file->isFile() || 'php' !== strtolower( $file->getExtension() ) ) {
continue;
}
if ( 1 === substr_count( $file->getFilename(), '.' ) ) {
continue;
}
if ( false === strpos( rocket_direct_filesystem()->get_contents( $file->getPathname() ), '$rocket_cookie_hash' ) ) {
continue;
}
rocket_direct_filesystem()->delete( $file->getPathname() );
}

More info: #5854

@vmanthos
Copy link
Contributor

vmanthos commented Apr 7, 2023

@CrochetFeve0251 I continued exploratory testing on my Hostinger test server. They have a staging site feature where both the URL and the files paths are different compared to the production site.

I pushed changes from staging to production and I found that they are not overwriting the value of wp_rocket_last_base_url. So in both environments, it remains the same after the push and this prevents rocket_domain_changed from firing. Consequently, anything that relies on this is failing.

I discovered that while they are overwriting the database, they won't do it for fields whose values contain the site's URL, e.g. wpr_rocket_cache rows. I'm guessing other hosts will do the same to prevent breaking the website, e.g. if replacing the siteurl or home options in the wp_options table.

To circumvent this we can save a hash of the URL of the site to wp_rocket_last_base_url instead of the actual URL.

What do you think?

@vmanthos
Copy link
Contributor

vmanthos commented Apr 7, 2023

@CrochetFeve0251 On Hostinger, .htaccess isn't pushed from staging to production. This could be happening with other hosts as well and the reason is likely similar to the "why" they are not overwriting database entries that contain the URL of the site.

If there are changes to the staging site, e.g. a URLis add to the "Never Cache URLs", and our rewrite rules are in place, after the push those changes will not be reflected on the .htaccess file of the production site until the file is regenerated. Therefore the excluded URL, if previously cached, will still be served.

We should flush_rocket_htaccess() when rocket_domain_changed fires. That can be added here:

public function regenerate_configs() {
rocket_generate_advanced_cache_file();
}


To sum up my findings so far:

  1. rocket_clean_domain() runs but the URL we pass to be cleared is the current one, i.e. the one after the change. (comment)
  2. The logic about which .php files are cleared in the configuration folder must be changed. (comment)
  3. We need to store a hash(?) of the site's URL in wp_rocket_last_base_url. (comment)
  4. The .htaccess file should be regenerated when we detect a domain change.

@vmanthos
Copy link
Contributor

vmanthos commented Aug 1, 2023

@piotrbak I'm currently testing the latest commit to see if the reported issues are fixed.

@vmanthos
Copy link
Contributor

vmanthos commented Aug 2, 2023

@piotrbak In multisites, the notice is still displayed but the button to regenerate the files and clear the cache is missing:
image

Is this the expected outcome or shouldn't we display the notice at all?

@vmanthos
Copy link
Contributor

vmanthos commented Aug 2, 2023

@CrochetFeve0251 Can you please check Remy's comment?
#5688 (comment)

Currently, the URL leads directly to the EN documentation, i.e. doesn't open in the Beacon, and will be the same regardless of the language the user is using, e.g. French.

@vmanthos
Copy link
Contributor

vmanthos commented Aug 4, 2023

@CrochetFeve0251 The following issues have been fixed:

  • Links aren't displayed in Beacon.
  • Notice is displayed in multisites.

However, now the button to regenerate WP Rocket's files after a "push" isn't displayed. I believe it's related to the removal of $args['action'] = 'regenerate_configuration'; in the following commit:
7c67bb3#diff-9055161c7bbe500619682e326d32655472afc64ecaae55868d5e0225d330f53aL192-L195

Can you please look into this? 🙏

Copy link
Contributor

@vmanthos vmanthos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @CrochetFeve0251 for your continuous effort on this PR.

This is working as expected! 🎉

TestRail report: testrail-report-511.pdf

@vmanthos vmanthos added this to the 3.14.4 milestone Aug 4, 2023
@vmanthos vmanthos added this pull request to the merge queue Aug 4, 2023
Merged via the queue into develop with commit 9019115 Aug 4, 2023
7 of 8 checks passed
@vmanthos vmanthos deleted the enhancement/5667-detect-domain-change branch August 4, 2023 08:17
@vmanthos
Copy link
Contributor

@piotrbak @wp-media/php

While testing another PR I found that the notice will be displayed when:

  1. A translation plugin is used, e.g. WPML.
  2. The setting to display or not the default language's code in the permalink is enabled/disabled.

For WPML, we are already handling cache clearing when the specific option changes.

With the notice displayed, the WMPL users who will click the "Regenerate WP Rocket configuration files now" button will have the cache cleared again and additionally, the advanced-cache.php and .htaccess files will be regenerated.

How should we handle this?

@piotrbak
Copy link
Contributor

@vmanthos Are there any changes in our config file (new one generated when doing a visit) or .htaccess (change in rewrite rules after regenerating it) when changing this option in WPML?

@vmanthos
Copy link
Contributor

The only change I see is in the rewrite rules:
RewriteBase / becomes RewriteBase /en/ and vice versa depending on whether the option to display /en/ is enabled or disabled.

@piotrbak
Copy link
Contributor

@vmanthos So there's a value of doing it 🤔

@vmanthos
Copy link
Contributor

If there is, then we missed something when we added the fix for this:
#5761

In any case, this was a random finding and it would be good to investigate it further to make sure:

  1. If it's indeed useful.
  2. If it can cause side effects.

@piotrbak
Copy link
Contributor

Probably the home_url or site_url is changed when doing this operation. Now I'm thinking, rewrite rules might not be working correctly for different languages if they're set to /en/, but that's not a regression, since it'd happen when regenerating htaccess anyways.

@piotrbak
Copy link
Contributor

@CrochetFeve0251 Do you think it'd possible not to display the message based on the action that triggered it?

@CrochetFeve0251
Copy link
Contributor Author

@piotrbak what do you mean by action? I need some context.

@piotrbak
Copy link
Contributor

@CrochetFeve0251 It's about this one:

The setting to display or not the default language's code in the permalink is enabled/disabled.

So, we're on the WPML Languages Settings page and we're changing this one:
image

Pinging @vmanthos for confirmation

@vmanthos
Copy link
Contributor

Exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display a notice after a site was cloned to disable/re-enable WP Rocket
4 participants