Open tianon opened 10 months ago
What I think we need to resolve in order to merge this:
a new test to make sure it works correctly (and continues to do so)
figure out what to do about the $is_nginx
hack I added in wp-config
-- that's not a real long-term solution and if we want to merge this, we should have a conversation with the WordPress community first (or find where one has already happened and implement any more official workarounds than my personal hack)
For 1., I guess that's really adapting the existing apache
test to be slightly more generic and assigning it to both variants.
That's pretty cool @tianon! Paging @javorszky for assistance - I believe they have been involved with Unit/Wordpress integration lately on our team.
@tianon hello! I'll be on this on Monday morning GMT timezone :) I'm super excited about this!
Here's the simplified version of my current hack (for you to review Monday :heart:), since this PR probably isn't the easiest thing in the world to read. :joy:
I'm essentially injecting this at the end of wp-config.php
(because it includes wp-settings.php
which includes vars.php
which is what sets $is_nginx
, so if I put it at the end, I can supplement that value with additional detection without patching WordPress itself):
// ...
/** Sets up WordPress vars and included files. */
require_once ABSPATH . 'wp-settings.php';
// WordPress does not support NGINX Unit (yet), so we have to do a little hackery to let it know that Unit supports nice permalinks (and prevent it from insisting on injecting "/index.php" in all the permalink options) -- the simplest way is teaching it to recognize Unit as if it were NGINX (which is only semi-true, but true enough for these purposes)
$is_nginx = $is_nginx || str_starts_with($_SERVER["SERVER_SOFTWARE"], "Unit/");
// see also https://github.com/WordPress/WordPress/blob/39f7f558d91afdd2f3afc7f3b049a6a800cd3f80/wp-includes/vars.php#L127
More useful backlinks (ie, how I ended up at the conclusion I needed to patch $is_nginx
):
https://github.com/WordPress/WordPress/blob/32bd6d1a8e2c374cb6d4caf3102f7ba47dd2de9c/wp-admin/options-permalink.php#L79-L81 (where /index.php
is force-injected into all permalink options in the settings)
https://github.com/WordPress/WordPress/blob/32bd6d1a8e2c374cb6d4caf3102f7ba47dd2de9c/wp-admin/includes/misc.php#L46 (the implementation of got_url_rewrite
which amounts to "do we have mod_php
in Apache OR are we running in nginx OR are we in IIS7 with permalinks")
I guess a more "upstream-friendly" hack/workaround would be implementing a proper got_url_rewrite
filter, but I'm not sure whether I can inject one of those just via wp-config.php
or whether that'd have to be a proper plugin (which also isn't a bad idea for non-Docker users which we could recommend users of this image install instead of my hacks). Of course, I still think the ideal solution is probably fixes in WordPress itself to handle this case correctly, but I don't know whether that's already been attempted or whether there's any upstream appetite for that. :sweat_smile:
Neat, I had some time to look into this. From what I understand this repository is responsible for generating the individual Dockerfiles for WordPress at the different tags.
There are three immediate unit-specific observations:
processes: 2
or
"processes": {
"idle_timeout": 30,
"max": 50,
"spare": 1
}
See https://github.com/nginx/unit/issues/1041#issuecomment-1884837237 for reasoning for that one. The important point is that the number of processes should be more than 1
main
branch and the POC branch for the issue-916 fix, is absolutely totally fine with pretty permalinksFor upstream work that we need the WordPress folks, I think we need to add two things:
--control
flagHey @tianon,
Small update on where things are on our side:
This PR is an active item in our planning and we keep an eye on it 🙂
The Unit configuration I've included here is based on the example provided by the Unit documentation (https://unit.nginx.org/howto/wordpress/), although I did spend time understanding it before blindly trusting it. 😄
The biggest 😬 here (IMO) is that WordPress itself does not know about Unit yet, so the permalink handling is a bit off -- I've added a hack in
wp-config-docker.php
(specifically in the Unit images only) which works around it by teaching WordPress that Unit is effectively NGINX (which does work and is accurate enough for what WordPress needs), but that should probably be an issue or PR to the WordPress community instead.(FYI @thresheek - maybe you have more context or know someone who does about WordPress recognizing Unit natively and not injecting
/index.php
into permalinks when it's the server software? :eyes:)