Blueprints: Define constants in auto_prepend_file, silence warnings related to redefining those constants - #1400
Merged
Conversation
…elated to redefining those constants Changes the default meethod of defining PHP constants from rewriting the `define()` calls in wp-config.php to pre-defining constants via an `auto_prepend_file`. ## Problem solved by this PR Playground needs to define some WordPress constants like SITE_URL or HOME_URL. Unfortunately, the site loaded into Playground may already have a conflicting `define()` call in its `wp-config.php`. Historically, we'va avoided PHP warnings by rewriting the `define()` calls in the `wp-config.php` file. It works quite well, but when the `wp-config.php` file is mounted from a local directory, Playground would change its contents. This is typically not what the user wants. These changes would show up in `git commit` and would be annoying at best, or get accidentally commited to the repository at worst. ## Implementation Instead of rewriting the `wp-config.php`, this PR changes the default behavior to defining the constants in a pre-loaded PHP file. This triggers PHP warnings, but we're silencing them using a custom error handler. As a result, Playground constants take precedence over custom define() calls in a way that doesn't trigger warnings. This is an easy and hacky way of implementing this. A better solution would be a PHP.wasm-level patch to enable "warning-less constants" that can't be redefined but the conflicting `define()` call fails silently. ## Testing instructions Confirm the unit tests pass
adamziel
added
[Type] Enhancement
Collaborator
Author
|
E2E failures are consistent with trunk, everything else seems to work. @brandonpayton @bgrgicak does this change seem fine? |
| it('should not raise a warning when conflicting with a user-defined constant', async () => { | ||
| // Preload the warning-silencing error handler | ||
| await enablePlatformMuPlugins(php); | ||
| await preloadRequiredMuPlugin(php); |
Member
There was a problem hiding this comment.
nitpicking something that wasn't added in this PR:
Should this name be a plural preloadRequiredMuPlugins?
Collaborator
Author
There was a problem hiding this comment.
Perhaps! Also, I'm questioning whether these two functions should be separate :-)
brandonpayton
approved these changes
May 15, 2024
brandonpayton
left a comment
brandonpayton
left a comment
Member
There was a problem hiding this comment.
I read through the changes, and this looks fine to me.
Member
|
It looks good, in fact. :) |
Collaborator
|
Looks great! I'm really glad we managed to move this to internals. |
adamziel
added a commit
that referenced
this pull request
May 16, 2024
…e the original zip (#1412) Fixes a regression introduced in #1400 that caused the original zip file to be deleted. To test, confirm you can preview WordPress/wordpress-develop#6318 on this branch. Closes #1409
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes the default meethod of defining PHP constants from rewriting
the
define()calls in wp-config.php to pre-defining constants viaan
auto_prepend_file.Problem solved by this PR
Playground needs to define some WordPress constants like SITE_URL or HOME_URL.
Unfortunately, the site loaded into Playground may already have a
conflicting
define()call in itswp-config.php. Historically, we'vaavoided PHP warnings by rewriting the
define()calls in thewp-config.phpfile.It works quite well, but when the
wp-config.phpfile is mounted from alocal directory, Playground would change its contents. This is typically
not what the user wants. These changes would show up in
git commitandwould be annoying at best, or get accidentally commited to the
repository at worst.
Implementation
Instead of rewriting the
wp-config.php, this PR changes the defaultbehavior to defining the constants in a pre-loaded PHP file. This
triggers PHP warnings, but we're silencing them using a custom
error handler. As a result, Playground constants take precedence
over custom define() calls in a way that doesn't trigger warnings.
This is an easy and hacky way of implementing this. A better solution
would be a PHP.wasm-level patch to enable "warning-less constants" that
can't be redefined but the conflicting
define()call fails silently.Testing instructions
Confirm the unit tests pass