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

New Page Layout Picker: refresh page pattern cache when site lang changes (round 2) #51122

Merged

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Mar 16, 2021

This is round to after having to revert #51087 which caused fatals during deploy.

Changes proposed in this Pull Request

  • Change pattern cache key to use site locale instead of account/user locale
  • Update execution order to ensure the get_iso_639_locale() function is available when __construct() is called
  • Protect against fatals during deploy by not calling get_iso_639_locale() if the host still has the old version of full-site-editing-plugin.php that hasn't updated the execution order Refactor: Load ETK common code before the plugins_loaded action #51158 has taken care of these items

After changing the site language the layout names and categories in the page layout picker still show in the previous language. And when the pattern is inserted it inserts the pervious language. You'd have to wait a day after updating the site language before the picker would fix itself.

The problem is the pattern data is cached using a key that includes the account language, not the site language. This change makes sure the cache key is using the exact same locale slug that's being used in the query to get pattern data.

Testing instructions

  • install-plugin.sh etk update/refresh-page-pattern-cache-on-site-lang-change on sandbox
  • On sandbox site create a new page (layout picker will open)
  • In another tab update the site language
  • Refresh the editor tab, the modal updates to show pattern thumbnails, names and categories using the new language
  • Insert a layout, and the content added to the page should be in the new site language.

@matticbot
Copy link
Contributor

@p-jackson p-jackson requested review from a team March 16, 2021 23:51
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 16, 2021
@p-jackson p-jackson self-assigned this Mar 16, 2021
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@p-jackson
Copy link
Member Author

The php unit tests are failing because of this issue in wp-env WordPress/gutenberg#29800 p1615778999014600-slack-C02DQP0FP

andrewserong
andrewserong previously approved these changes Mar 17, 2021
Copy link
Member

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

LGTM! And tested that it's using the site's language correctly (and that the fallback behaves as expected) 👍

Good to know about the issue during deployment and working around the priority for loading the common module. I suppose another approach / future enhancement could be moving some of the things out of common/index.php that don't need to be deferred until plugins_loaded has fired. I imagine a few of those functions (like get_is_639_locale) could be defined outside of a hook, but that's a refactor outside the scope of this change, of course 🙂

Thanks for improving how the language is loaded and cached!

@p-jackson
Copy link
Member Author

Ah thanks for that @andrewserong! I'll merge as is since it's tested now, but now that you mention it, moving get_is_639_locale so it's not dependent on plugins_loaded makes a lot more sense. It's more straightforward than fiddling with priorities.

Scratch that, I think I'm going to change approaches.

Even after I do the cleanup PR and remove the function_exists call, we'll still be left with priority fiddling. Moving common code to a place that doesn't depend on plugins_loaded will still require two deploys, but once the second deploy is done I think the code will be left in a tidier place.

🙇

@p-jackson p-jackson added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 17, 2021
@p-jackson p-jackson dismissed andrewserong’s stale review March 17, 2021 21:26

I'm changing approaches

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 17, 2021
@p-jackson
Copy link
Member Author

PR that loads common code outside of the plugins_loaded action is ready for review #51158

I will refactor this PR to be based off #51158 and simplify it since we can assume #51158 will ship first 👍

@andrewserong
Copy link
Member

Perfect, thanks for following up with that @p-jackson!

The page pattern cache was keyed on the account language. However the
account language isn't used when making the request to get the page
pattern data. Switching to the site's language (i.e. the same locale
slug used when fetching pattern data) means the cache will be
invalidated correctly.
@p-jackson p-jackson force-pushed the update/refresh-page-pattern-cache-on-site-lang-change branch from e70391b to 2f41e84 Compare March 18, 2021 00:16
Copy link
Member

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

LGTM, just gave it another quick smoke test 👍

@p-jackson
Copy link
Member Author

Tré cool 🎉 Thanks for the help all

@p-jackson p-jackson merged commit cebdbfa into trunk Mar 18, 2021
@p-jackson p-jackson deleted the update/refresh-page-pattern-cache-on-site-lang-change branch March 18, 2021 00:48
@matticbot matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants