-
Notifications
You must be signed in to change notification settings - Fork 41
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-now: Add support for custom URL's #78
wp-now: Add support for custom URL's #78
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.
@sergeymitr Thanks for the PR!, I'll try it later.
The code looks good so far, I just wonder if customport
is necessary.
Another approach to change the siteurl
could be accepting blueprints.
packages/wp-now/src/run-cli.ts
Outdated
.option('siteurl', { | ||
describe: | ||
"Custom site URL to access the site ('home' and 'siteurl' option values).", | ||
type: 'string', | ||
}) | ||
.option('customport', { | ||
describe: | ||
"Custom port number. Needed if you're using a tunneling service (e.g. ngrok), usually 80.", | ||
type: 'number', |
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 wonder if we can/should remove customport
and consider it always part of siteurl
.
Assuming that if a given siteurl
is passed the custom port will be 80
and if it's a different port, it needs to be explicit on siteurl
string.
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'd prefer this too.
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 for taking a look!
The problem with customport
is that local custom domains (the hosts
file ones) don't do port forwarding, so we'll need to add the port explicitly to the custom domain, e.g. http://somethingcool.local:8881
. Since the port can differ depending on whichever's open, it cannot be passed as an option.
Tunneling services, on the other hand, do port forwarding, so http://somethingcool.ngrok.something
(port 80) will forward to http://localhost:8881
. So the port should be in the URL for local custom domains, and shouldn't be there for non-custom domains.
At the very least we would need a flag, something like --addPortNumberToTheSiteURL
to determine whether to add that wp-now-determined port number to the site URL or not.
I can imagine the situation when tunneling service port differs from 80, that's why I made it a port number and not a flag.
I didn't manage to come up with a way to get rid of it altogether, so if I'm missing something, please let me know.
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.
After thinking about it, I guess we could take that port number from the URL, and get rid of the customport
option.
That would mean 3 possible states:
- port 80: no port in the final URL for tunneling service
- non-80 port specified: the final URL will contain that port for tunneling service with non-80 port for some reason
- no port: port will be automatically added to the URL by wp-now
The whole situation is a bit user-unfriendly though, both with customport
option and without it.
May be we should replace it with a flag (e.g. --proxying
) to indicate that the final URL does not need to get the port added to it?
That wouldn't work for proxying via non-80 ports, but I don't even know if that's a thing. That potential flexibility may not be worth the added complexity.
What do you think about that?
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.
The more that I think about this, the more I feel we should solve this with Blueprints (#9).
@adamziel Could you provide a sample Blueprint that would achieve what @sergeymitr wants?
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 @adamziel.
After additional research, I don't think blueprints would solve the issue entirely:
- port number is being determined dynamically later in the code, so we'd still need to edit the
getWpNowConfig()
function to add it to the URL - decision whether to add it to the URL (for local domains) or not (for reverse proxies/tunneling services)
Considering changes in the code will still be needed, blueprints don't bring significant improvement.
Since you're striving toward keeping the tools lean (I fully support this), I suggest one of the approaches below.
I believe they would bring the needed flexibility to solve a lot of edge cases that users will come up with:
- Do not define the constants
WP_HOME
andWP_SITEURL
, and save them in the database options table just like regular WordPress installations do. - Depending on a flag (e.g.
--extract-url-config
) or some blueprint configuration, addWP_HOME
andWP_SITEURL
into the existingwp-config.php
, or create a new one. - Use a different flag (e.g.
--skip-url-config
) or a blueprint to skip settingWP_HOME
andWP_SITEURL
. Thus, the site will not work until they are defined in a customwp-config.php
. - Use another flag (e.g.
--auto-url-config
) or a blueprint to makeWP_HOME
andWP_SITEURL
take whatever the value is in the environment variables (example from Jetpack dev env).
It's likely that the issue of hardcoded WP_HOME
and WP_SITEURL
will eventually come up, as not being able to change the URL setting of the site limits the ability to develop and test quite a bit. Right now the wp_option
records get ignored, trying to customize it in wp-config.php
throws the "Constant already defined" error.
What do you think if we implement one of those approaches, or may be you have other ideas?
I believe being able to edit those values via wp-config.php
as intended by WordPress would be a better approach than having to implement and apply blueprints to do the same.
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 believe being able to edit those values via wp-config.php as intended by WordPress would be a better approach than having to implement and apply blueprints to do the same.
@sergeymitr, I totally agree. The more we keep the natural behaviour, the best.
Currently, those config variables are set here.
playground-tools/packages/wp-now/src/wp-now.ts
Lines 327 to 337 in 6602715
const wpConfigConsts = { | |
WP_HOME: siteUrl, | |
WP_SITEURL: siteUrl, | |
}; | |
if (wordPressVersion !== 'user-defined') { | |
wpConfigConsts['WP_AUTO_UPDATE_CORE'] = wordPressVersion === 'latest'; | |
} | |
await defineWpConfigConsts(php, { | |
consts: wpConfigConsts, | |
virtualize: true, | |
}); |
It's possible to apply and overwrite those variables with a blueprint step. Which is very similar to your second option:
Depending on a flag (e.g. --extract-url-config) or some blueprint configuration, add WP_HOME and WP_SITEURL into the existing wp-config.php, or create a new one.
I already created a PR to support the blueprints. I still need to validate your case, and I'm confident that it will fix your use case 🤞.
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.
Hi @sejas.
Speaking of my task at hand, the blueprints would allow using tunneling services, so we're good here.
However, I don't think they'll solve the issue entirely.
WP-now starts a server on a random port (even if you provide port number, it may change if the port is closed). Therefore, custom local URL's will look like this: http://something.local:8765
. Since we cannot know the port number before starting the server, we'll still need to edit the code to append the port number to the URL provided in the blueprint.
However, tunneling services (ngrok, JT) proxy requests straight to that custom port, making you access the site via port 80. So, the website URL will look like http://something-ngrok.tld
, with no port number needed.
Blueprints should work for the tunneling services, since there's no need to add port number to the site URL. However, they will not work for regular custom domains because we need to know the port in advance, or append it to the URL. If we go with the latter, we also need to somehow inform the script whether we actually need to append the port.
It also limits the users because of the non-native way to define the site URL. One thing that comes to mind is running multi-URL sites. While wp-config.php
allows us to simply use environment variables to make the site work on any domain, blueprints don't do that.
They'll also need to restart the server every time the domain changes.
Blueprints are undoubtedly a great way to customize the server, but I still think making WP_HOME
and WP_SITEURL
immutable is quite limiting.
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.
- @sergeymitr I merged wp-now: execute blueprint steps given a file path #82 , It's working on Node v20. Feel free to test it. We didn't release the package yet.
WP-now starts a server on a random port (even if you provide port number, it may change if the port is closed). Therefore, custom local URL's will look like this: http://something.local:8765. Since we cannot know the port number before starting the server, we'll still need to edit the code to append the port number to the URL provided in the blueprint.
The default port is not random, it's usually 8881
. You are right that this port could change if that port is already in use, but it increases sequentially. So it's a deterministic way and users can predict what port will run wp-now.
Since the port is predictable, the user can write the given port manually and include it on WP_SITEURL through a blueprint.
I'm still thinking that passing the --url
parameter is something that can be handy for wp-now, but this url
can also include the default port or the same port passed to --port
e.g. --url=http://myurl.wp-now:8881
or --url=http://myurl.wp-now --port=80
Blueprints are undoubtedly a great way to customize the server, but I still think making WP_HOME and WP_SITEURL immutable is quite limiting.
I won't be available next week, but we can keep iterating this PR and probably accept --url
for a fast option change.
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.
Hi @sejas.
Your blueprint PR works great, thanks for implementing that!
It would come handy if we could edit WP_HOME
and WP_SITEURL
right in wp-config.php
, but that's no longer a priority since everything works with blueprints.
I'm going to close the PR now as the changes no longer necessary, and we can revisit this later if needed.
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.
Rather than introducing a new argument, I wonder if we'd be better off incorporating Blueprint support (#9) and then using the Blueprint to set these values.
packages/wp-now/src/run-cli.ts
Outdated
.option('siteurl', { | ||
describe: | ||
"Custom site URL to access the site ('home' and 'siteurl' option values).", | ||
type: 'string', | ||
}) | ||
.option('customport', { | ||
describe: | ||
"Custom port number. Needed if you're using a tunneling service (e.g. ngrok), usually 80.", | ||
type: 'number', |
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'd prefer this too.
What?
Support for custom URL's and tunneling services (e.g. ngrok).
Why?
Useful for development, requested by users.
How?
We add two new CLI options:
--url
replacesWP_HOME
andWP_SITEURL
values for the site to support custom domain names--customport
is needed for proxying tunneling services (e.g. ngrok)For local custom domains, it's enough to add the site record into the
hosts
file:And then pass that value as
--url="http://wpnow-test.local"
The website will be available at
http://wpnow-test.local:[port]
, with[port]
being the open port used by wp-now in regular fashion.The final URL will be displayed in shell and loaded automatically in browser instead of the localhost one:
If you use ngrok or similar tunneling service, you will need to pass both
--url="http://your-ngrok-domain"
and--customport=80
because proxying to 8881 (or whatever is generated by wp-now) happens automatically.Also, there's still no HTTPS supported, so make sure you use
http://
for whatever domain you're proxying.Testing Instructions
Local custom domain
hosts
file (e.g.127.0.0.1 wpnow.local
)Tunneling service
ngrok http localhost:8882 --scheme=http
(no HTTPS supported yet)nx preview wp-now start --port=8882 --url="http://your-ngrok-domain.something" --customport=80
curl -i http://your-ngrok-domain.something