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.

Sign up for GitHub

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

Jump to bottom

Blueprints: Wrap define() calls in if(!defined()) in wp-config.php #901

Closed
wants to merge 1 commit into from
Closed

Blueprints: Wrap define() calls in if(!defined()) in wp-config.php #901

wants to merge 1 commit into from

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Dec 23, 2023 •
edited
Loading

Rewrites wp-config,.php and wraps all define() calls in a conditional if(!defined()). The goal is to avoid conflicts between the constants defined via the defineWpConfigConsts Blueprint step and the ones defined in wp-config.php.

Related to WordPress/playground-tools#135

Open questions

This PR changes the contents of wp-content.php which is fine in the in-browser Playground environment, but may be unexpected when Playground is used for local development e.g. via local Directory Sync or wp-now. In particular, these changes would show up in git diff and could confuse the developer. One workaround could be to also add an explanatory comment like:

// These changes were automatically added by WordPress Playground because
// you used the defineWpConfigConsts Blueprint step to define a constant with
// the same name as one already declared this file (WP_DEBUG).

However, I don't see any other solution here. If a constant is already defined, the define() call will issue a warning. Therefore, we must avoid conflicting define() calls in wp-config.php.

Two alternative ideas:

  1. Wait until the defineWpConfigConsts() step is called and only then wrap the specific constants definitions that would trigger a warning.
  2. Refactor the defineWpConfigConsts() step either add a define() call in wp-config.php or rewrite the second argument of an existing define() call. This makes the constant values apparent as all the values are available for inspection in the single location that the developer already knows about and will inspect for debugging. Also, it solves the problem of including the Blueprint-defined constants in the exported Playground.

Either way, we're rewriting wp-config.php as we cannot leave the conflicting define() call as it is.

Thinking about it more, I'm leaning towards closing this PR and exploring the second alternative above. It wouldn't handle dynamic calls like define($wpdebug, true), but it could wrap them in an if() and add an inline comment to explain the wrapping.

cc @dmsnell @danielbachhuber @swissspidy

Testing Instructions

Go to the following URL without this PR applied and confirm there are warnings like Warning: Constant WP_DEBUG already defined:

http://localhost:5400/website-server/#{%22landingPage%22:%22/%22,%22phpExtensionBundles%22:[%22kitchen-sink%22],%22preferredVersions%22:{%22php%22:%228.0%22,%22wp%22:%225.9%22},%22steps%22:[{%22step%22:%22defineWpConfigConsts%22,%22consts%22:{%22WP_DEBUG%22:true}},{%22step%22:%22defineWpConfigConsts%22,%22consts%22:{%22WP_DEBUG_ONLY%22:%22a%22}}]}

Now go there with this PR applied and confirm the warnings are gone.

@adamziel
Wrap wp-config define() calls in if(!defined())
58744ec 
Wraps all define calls in a conditional `if(!defined())` to ensure that the default
wp-config.php constants will not class with the ones defined with the `defineWpConfigConsts`
Blueprint step.

Related to WordPress/playground-tools#135
@adamziel adamziel changed the title Wrap wp-config define() calls in if(!defined()) Blueprints: Wrap define() calls in if(!defined()) in wp-config.php Dec 23, 2023
@adamziel adamziel mentioned this pull request Dec 26, 2023
Merged
@adamziel
Copy link
Collaborator Author

Closing in favor of #902

@adamziel adamziel closed this Dec 27, 2023
adamziel added a commit that referenced this pull request Jan 8, 2024
@adamziel
Blueprints: Rewrite wp-config.php to define constants in the defineWp…
88b6a6b 
…ConfigConsts step (#902)

## Summary

This PR enhances the defineWpConfigConsts step by allowing constants to
be defined in two ways:

1. By rewriting wp-config.php (default behavior)
    * Existing define() calls are rewritten with new constant values
    * New define() calls are prepended
    * All calls wrapped in if(!defined()) to avoid conflicts
2. By calling `define()` before script execution (previous behavior)
    * Constants defined directly via PHP before script execution
    * Does not modify wp-config.php
    * May conflict with existing defines in wp-config.php (limitation)

Related to #901
Related to WordPress/playground-tools#135
Closes #900

## wp-config.php rewriting details 

The following `wp-config.php`:

```php
<?php
define('WP_DEBUG', true);
// The third define() argument is also supported:
define('SAVEQUERIES', false, true);

// Expression are wrapped in `if(!defined())` guards
define(true ? 'WP_DEBUG_LOG' : 'WP_DEBUG_LOG', 123);

// Guarded expressions shouldn't be wrapped twice
if(!defined(1 ? 'A' : 'B')) {
    define(1 ? 'A' : 'B', 0);
}

// More advanced expression
define((function() use($x) {
    return [$x, 'a'];
})(), 123);
```

When rewritten like this:

```php
rewrite_wp_config_to_define_constants($content, [
    'WP_DEBUG' => false,
    'WP_DEBUG_LOG' => true,
    'SAVEQUERIES' => true,
    'NEW_CONSTANT' => "new constant",
]);
```

Will become:

```php
<?php
define('WP_DEBUG_LOG',true);
define('NEW_CONSTANT','new constant');
?><?php
define('WP_DEBUG',false);
// The third define() argument is also supported:
define('SAVEQUERIES',true, true);

// Expression are wrapped in `if(!defined())` guards
if(!defined($const ? 'WP_DEBUG_LOG' : 'WP_DEBUG_LOG')) {
     define($const ? 'WP_DEBUG_LOG' : 'WP_DEBUG_LOG', 123);
}

// Guarded expressions shouldn't be wrapped twice
if(!defined(1 ? 'A' : 'B')) {
    define(1 ? 'A' : 'B', 0);
}

// More advanced expression
if(!defined((function() use($x) {
   return [$x, 'a'];
})())) {
    define((function() use($x) {
        return [$x, 'a'];
    })(), 123);
}
```

## Testing instructions

Go to the following URL without this PR applied and confirm there are
warnings like Warning: Constant WP_DEBUG already defined:


http://localhost:5400/website-server/#{%22landingPage%22:%22/%22,%22phpExtensionBundles%22:[%22kitchen-sink%22],%22preferredVersions%22:{%22php%22:%228.0%22,%22wp%22:%225.9%22},%22steps%22:[{%22step%22:%22defineWpConfigConsts%22,%22consts%22:{%22WP_DEBUG%22:true}},{%22step%22:%22defineWpConfigConsts%22,%22consts%22:{%22WP_DEBUG_ONLY%22:%22a%22}}]}

Now go there with this PR applied and confirm the warnings are gone. The
Blueprint encoded in the URL above calls the `defineWpConfigConsts` step
twice just to make sure a duplicate and conflicting `define()` call
won't be added.

cc @sejas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
[Package][@wp-playground] Blueprints [Type] Developer Experience [Type] Enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@adamziel