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

Refactor: Load ETK common code before the plugins_loaded action #51158

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

p-jackson
Copy link
Member

Changes proposed in this Pull Request

common/index.php contains helper functions that would be useful when other ETK modules are being setup during the plugins_loaded action. Unfortunately they can't be used though, because those helper functions are defined as part of plugins_loaded too.

There's nothing in common/index.php that means loading it should be delayed. So moving the require_once call so that it gets loaded as soon as full-site-editing-plugin.php is loaded.

Context: as part of #51122 I want to be able to call get_iso_639_locale() during Starter_Page_Templates's __construct(), so I want to guarantee that get_iso_639_locale() gets defined first.

Testing instructions

  • install-plugin.php etk update/etk-load-common-code-earlier
  • Smoke test ETK, it's functions should still load (new page modal, launch flows, etc.)
  • Verify that the common js/css assets are still enqueued (e.g. check for hide-plugin-buttons-mobile.css in the network tab)

`common/index.php` contains helper functions that would be useful when
other ETK modules are being setup during the `plugins_loaded` action.
Unfortunately they can't be used though, because those helper functions
are defined as part of `plugins_loaded` too.

There's nothing in `common/index.php` that means loading it should be
delayed. So moving the `require_once` call so that it gets loaded as
soon as `full-site-editing-plugin.php` is loaded.
@p-jackson p-jackson requested review from a team March 17, 2021 23:17
@p-jackson p-jackson self-assigned this Mar 17, 2021
@matticbot
Copy link
Contributor

@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 17, 2021
Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

This makes perfect sense to me, and it tests well. It doesn't really make a difference on JS/CSS loading order either, since those still use normal hooks.

@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

😎 thanks @noahtallen
I thought you might have been interested in this from a workflow point of view. This got a fast review so probably won't matter now, but having two PRs, where the second one's branch is based off the first one, is a way we can make this sort of things easily testable. Using install-plugin.php on the second PR will allow us to test the whole feature together like we generally do when making wpcom changes.

@noahtallen
Copy link
Contributor

Using install-plugin.php on the second PR will allow us to test the whole feature together like we generally do when making wpcom changes.

That's super cool, I don't think I'd considered that workflow. But it's really nice that it works 😎

@p-jackson p-jackson merged commit 6f8ff3f into trunk Mar 17, 2021
@p-jackson p-jackson deleted the update/etk-load-common-code-earlier branch March 17, 2021 23:58
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 18, 2021
@autumnfjeld
Copy link
Contributor

@p-jackson Just a lil heads up that it's install-plugin.sh not install-plugin.php. 😄 Took me a bit of what-am-i-doing-wrong and searching when I did a previous review with that also had the erroneous .php extension.

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.

4 participants