Conversation
|
@adamziel commenting here to be assigned as a reviewer. |
There was a problem hiding this comment.
Pull Request Overview
This PR makes WordPress polyfills optional by requiring explicit activation through a polyfill_wordpress_apis() function call, addressing a runtime conflict where polyfills were loaded when WordPress classes were not yet available but would be loaded later.
- Move all WordPress polyfill logic from autoload-time execution to an explicit function
- Update bootstrap.php to call the polyfill function for backward compatibility
- Add documentation explaining when and how to use the polyfill function
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| components/Polyfill/wordpress.php | Wrap existing polyfill code in polyfill_wordpress_apis() function |
| bootstrap.php | Add call to polyfill_wordpress_apis() to maintain existing behavior |
| README.md | Document the new polyfill function usage for non-WordPress environments |
| * Polyfills WordPress core functions for running in non-WordPress environments | ||
| * Polyfills WordPress core functions to support running the php-toolkit libraries | ||
| * in non-WordPress environments. | ||
| */ |
There was a problem hiding this comment.
The function polyfill_wordpress_apis() lacks documentation. Add a docblock explaining its purpose, when it should be called, and any important considerations about calling it multiple times or in different contexts.
| */ | |
| */ | |
| /** | |
| * Polyfills WordPress core functions and classes to enable the use of php-toolkit libraries | |
| * in non-WordPress environments. | |
| * | |
| * This function should be called during the initialization phase of a non-WordPress | |
| * environment to ensure compatibility with WordPress-dependent libraries. | |
| * | |
| * Important considerations: | |
| * - Calling this function multiple times is safe, as it checks for the existence of | |
| * functions and classes before defining them. | |
| * - Avoid calling this function in a WordPress environment, as it may conflict with | |
| * existing WordPress core implementations. | |
| */ |
| $GLOBALS['_doing_it_wrong_messages'] = array(); | ||
| function _doing_it_wrong( $method, $message, $version ) { | ||
| $GLOBALS['_doing_it_wrong_messages'][] = $message; | ||
| function polyfill_wordpress_apis() { |
There was a problem hiding this comment.
The function doesn't protect against multiple calls. Consider adding a guard to prevent re-execution if the polyfills have already been loaded, which could cause 'function already exists' errors.
| function polyfill_wordpress_apis() { | |
| function polyfill_wordpress_apis() { | |
| static $already_loaded = false; | |
| if ( $already_loaded ) { | |
| return; | |
| } | |
| $already_loaded = true; |
php-toolkitrelies on WordPress APIs, such asapply_filters(). Sometimes they're missing, e.g. in PHPUnit runtime, and they're implicitly polyfilled when includingvendor/autoload.php:This polyfilling technique assumes the library is either:
However, it does not account an important third scenario that WooCommerce ran into:
wp-load.phpis includedSolution
With this PR, polyfills are no longer implicitly loaded as a part of the autoload sequence. Instead, we expose an explicit
polyfill_wordpress_apis()function that must be called by the library consumer.Rationale
At the time of
require "vendor/autoload.php", we don't know what the developer is going to do next. Maybe they'll includewp-load.php, in which case we must not load the polyfills. Or maybe then won't includewp-load.php, in which case we must load the polyfills. Exposing an explicitpolyfill_wordpress_apis()function lets them decide.The one downside is: it creates a gotcha. Anyone who wants to consume
php-toolkitas a library must know about that function and must call it, otherwise they'll see a fatal error. I'm noodling on adding an explicit warning to communicate this problem directly in the CLI without an additional google search.Testing plan
Confirm the CI tests pass – many
php-toolkitPHPUnit tests rely on WordPress polyfills and won't work ifpolyfill_wordpress_apis()is not called or is incomplete.