-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
wp-env: Add support for custom WP_HOME port #26507
wp-env: Add support for custom WP_HOME port #26507
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, just left one comment. I can merge after we figure that out
packages/env/lib/config/config.js
Outdated
baseUrl.port = config.env[ envKey ].port; | ||
|
||
// Don't overwrite the port of WP_HOME when set. | ||
if ( configKey !== 'WP_HOME' || ! baseUrl.port ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might as well apply this to all of the URLs we check here.
if ( configKey !== 'WP_HOME' || ! baseUrl.port ) { | |
if ( ! baseUrl.port ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @noahtallen. I'd be happy to make this change if you think it's best. The reason that I limited this to WP_HOME
is that I couldn't think of a legitimate reason why you would want your WP_SITEURL
or WP_TESTS_DOMAIN
to point to something other than the port for the WordPress environment. If you prefer limiting this to WP_HOME
I might rewrite that conditional to be a bit more readable, like:
if ( ! ( configKey === 'WP_HOME' && baseUrl.port ) ) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't think of a legitimate reason why you would want your WP_SITEURL or WP_TESTS_DOMAIN to point to something other than the port for the WordPress environment
Ok, that makes sense to me! If you'd like to make it more readable, that sounds good. I would also recommend a quick rebase to see if we can get the e2e tests passing!
This checks for whether a custom port has been set for `WP_HOME` in the .wp-env.json file before appending the environment's to support local environments where the front end needs to be pointed at another port (e.g., a headless app). Adds unit test. Resolves WordPress#26481
c765988
to
1f3b4e5
Compare
@noahtallen I've rewritten that conditional and rebased from the latest |
Yep, confirmed that these e2e failures are the ones that have been plaguing the master branch. Merging now! Thanks for the addition :) |
Description
This checks for whether a custom port has been set for
WP_HOME
in the .wp-env.json file before appending the environment's to support local environments where the front end needs to be pointed at another port (e.g., a headless app).Adds unit test.
How has this been tested?
Added unit tests. Also testing this config locally and ensured that the "Site Address (URL)" setting in Settings > General reflects the port that was set in my .wp-env.json file.
Types of changes
New feature (non-breaking change which adds functionality).
Resolves #26481.
Checklist: