Env: ensure correct port setting on related wp-config params

[pvogel2]

Description

Hi, I like the usage of wp-env for setting up a development environment for WordPress.
I recogniced when I configure WP_HOME and/or WP_SITEURL as property in the config prop of .wp-env.json or .wp-env.override.json the setting is not correctly propagated to the resulting wp-config.php.

That is because the port property setting in .wp-env.json or .wp-env.override.json is independend from WP_HOME and WP_SITEURL configuration but needs to be merged in the resulting parameters injected in wp-config.php.

I added a code block in env/lib/wordpress.js which modifies the value given to the docker-compose command so that the correct port setting is ensured.

How has this been tested?

All existing tests pass. Beside this it is an open question for me right now. Maybe I have not yet fully understood how the testing environment is used testing the wp-env stuff. There are no tests for setting wp-config.php config items right now as far as I can see. If you have any advice I would appreciate it.

I just tested by running the setup scripts and checking the resulting dev environment.

Example of test setup:
.wp-env.json:

{
  "port": 8888,
  "testsPort": 8889,
  "config": {
    "WP_DEBUG": true,
    "SCRIPT_DEBUG": true,
  }
}

.wp-env.overide.json:

{
  "config": {
    "WP_HOME": "http://test-on-internal-network.com/",
    "WP_SITEURL": "http://test-on-internal-network.com/",
    "WP_DEBUG": true,
    "SCRIPT_DEBUG": true,
  }
}

Expected for WordPress in wp-config.php

  ...
  define( 'WP_SITEURL', 'http://test-on-internal-network.com:8888/' );
  define( 'WP_HOME', 'http://test-on-internal-network.com:8888/' );
  ...

Expected for tests-WordPress in wp-config.php

  ...
  define( 'WP_SITEURL', 'http://test-on-internal-network.com:8889/' );
  define( 'WP_HOME', 'http://test-on-internal-network.com:8889/' );
  ...

but originally for both got

  ...
  define( 'WP_SITEURL', 'http://test-on-internal-network.com/' );
  define( 'WP_HOME', 'http://test-on-internal-network.com/' );
  ...

Types of changes

Bug fix

Checklist:

• My code is tested (by hand).
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[noahtallen]

Nice catch! This mostly makes sense to me.

Looking at line 83 in this same file, where we set --url=localhost:${ port }, do we need to update that to use the specified site url? (not sure how that mechanism works?

[noahtallen]

Do you think there's ever a possibility that WP_SITEURL would already include the port number? If so, should we still override it? (Honestly not sure what the expected behavior would be in this case.)

[pvogel2]

Hi noahtallen, thanks for the quick reply and your suggestions.

In fact I thought about similar things also and was not shure if the code change goes to the right direction.

I think the install setup, meaning '--url=localhost:${port} should stay as is. This is the WP default written to the database for home and siteurl option. And the port already comes from the config setting provided by the json files. So everything should be fine, or? By default no WP_HOME and WP_SITEURL is set inside wp-config.php. As far as I know it is common usage to rewrite this setting via wp-config.php if needed, and the database fallback is allways available if no WP_HOME or WP_SITEURL is set.

Rewriting the port setting if already set in WP_HOME or WP_SITEURL maybe can be problematic? I am not realy shure either. In fact these params are used to write out urls in the html pages. And it has to be the provided ports because docker maps only these ports to the containers.
Currently these port settings (if set in the WP_HOME and WP_SITEURL) would be configured for both instances, WordPress and also testsWordPress, which never can be correct, or?

Alternatively the rewrite only could be done when no port is set in the URL parameters (suggesting the author did not want to configure port 80 or port 443).

[pvogel2]

I just thought about some placeholder in the config param. Could it be a sollution? If an config author writes out the placeholder in the WP_HOME or WP_SITEURL parameter, it will be replaced with the port setting, otherwise nothing happens. Something like

{
  "config":  {
    "WP_HOME": "http://some.domain:${port}",
  "WP_SITEURL": "http://some.domain:${port}",
  }
}

and in wordpress.js then

  value = value.replace(/\$\{port\}/, port);

This should be backward compatible also.

[noahtallen]

Rewriting the port setting if already set in WP_HOME or WP_SITEURL maybe can be problematic

Yeah, exposing docker on the same port is definitely a requirement.

I just thought about some placeholder in the config param

Neat idea! My only concern about it is that I don't know how useful it would be outside of this use case, and it doesn't seem like we really need it that much anyways.

would be configured for both instances, WordPress and also testsWordPress, which never can be correct

True. This will be resolved by #22568. At least, a user could theoretically set it differently for each environment then.

[noahtallen]

I wonder if we should introduce a separate function for "merging" or "overriding" values in the config.

[pvogel2]

I like this idea. Especially for deep merging of .wp-env.json and .wp-env.override.json objects this would be nice.

[noahtallen]

I looked at adding a separate function in #22568 (for some different config values) so we can probably skip it here :)

[noahtallen]

I think this is a fine way to fix it for now :) I'll integrate it a bit more into my changes in #22568

[epiqueras]

If WP_HOME and WP_SITEURL will never have a different port than the images', then we shouldn't support specifying the port there. It should be appended from the config.