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

Hotfix: Change domain admin notice is showing when multilingual plugin is installed #6105

Merged
merged 6 commits into from
Aug 16, 2023

Conversation

engahmeds3ed
Copy link
Contributor

Description

Here we are adding a check that the change domain logic is happening only with admin UI not with any ajax request because when multilingual plugin is installed the home_url is changed in frontend leading to showing the notice in admin.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which improves an existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

Not groomed, it's a hotfix.

How Has This Been Tested?

Tested on the customer's site by @alfonso100.

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

@engahmeds3ed engahmeds3ed self-assigned this Aug 15, 2023
@engahmeds3ed engahmeds3ed marked this pull request as ready for review August 15, 2023 18:55
@engahmeds3ed engahmeds3ed requested a review from a team August 15, 2023 19:05
@@ -63,6 +64,10 @@ public static function get_subscribed_events() {
* @return void
*/
public function maybe_launch_domain_changed() {
if ( wp_doing_ajax() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure this is going to be enough for all i18n plugins we're compatible with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main problem is that we need to only execute this part of code in one place (frontend or backend) so even if the multilingual plugin is playing with the home url, it will play with it in frontend not backend, but we can let QA team validate this in every case, we tested it in the customer's site and it worked properly.

Copy link
Contributor

@CrochetFeve0251 CrochetFeve0251 Aug 16, 2023

Choose a reason for hiding this comment

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

@Tabrisrp that has a strong chance to work as I when I picked backend url it was cause translation plugins weren't modifying it.
It is true ajax front end request are calling ajax-admin.php.
Do you have any idea if the REST API is handled on the front or the backend? Cause it would be last thing I think that could modify the URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

@CrochetFeve0251 @engahmeds3ed @Tabrisrp, Unless it breaks something else, I would be in favor of releasing this version as hotfix quickly as it fixes the original issue reported.
If it can further be improved, that should definitely be done but if it needs longer investigation, maybe it can come after the hotfix?
@piotrbak any preference on your side?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MathieuLamiot I'd release it asap and then go with more hotfixes versions if needed. We knew that this update will be tricky but it's not possible to do exploratory testing for whole WP ecosystem.

We can do a quick tests for some language switchers of multilingual plugins until it passes the CR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@CrochetFeve0251, @engahmeds3ed, @Tabrisrp can we move forward with the CR quickly then, so that @Mai-Saad can do QA?
Thank you 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mai-Saad already started QA to validate it for all of our supported multilingual plugins.

Copy link

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

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

  • Reproducible for WPML and translate press => fixed with the PR
  • Not reproducible for Weglot and Polylang plugins
  • Notice is displayed when changing domain at config file and when cloning the website using the WPvivid plugin
  • If we update WPR while the notice is displayed, it will remain displayed till we click the regenerate btn. However, change language after that won't display the notice.
  • When clicking regenerate config btn after changing the domain name in wp-config, only the home page is preloaded after the cache clear (same on 3.14.4, further investigation is needed)

@Mai-Saad Mai-Saad added this pull request to the merge queue Aug 16, 2023
Merged via the queue into develop with commit 4f35023 Aug 16, 2023
8 checks passed
@Mai-Saad Mai-Saad deleted the hotfix/change-domain-notice branch August 16, 2023 15:35
@engahmeds3ed engahmeds3ed mentioned this pull request Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants