-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
Split is-installed
check into a non-multisite and a mulitsite specific one
#1388
Split is-installed
check into a non-multisite and a mulitsite specific one
#1388
Conversation
Can these be grouped better using Ansible blocks? They support block names too which can replace the comments. |
@swalkinshaw: The both checks for non-multisite and multisite are now grouped into Ansible blocks. |
One of the checks failed due to unrelated issues: Edit: Made a small change that triggered new tests that pass. |
Thanks so much for all this investigation @strarsis (and the PRs!). I agree this one makes the most sense for now at least 🚀 |
@strarsis Not sure if this is an issue on my end or not, but it only seems to work for singlesites; multisites still have the issue. |
For multisite this particular PHP warning is ignored, this preserves the way the constants are currently set and allows deploys. The PHP warning is caused by setting the constant to null. WordPress core since 6.0 doesn't handle this constant being null/empty. This would be the underlying issue. 1. Does that constant still have to set to null? 2. Can a non-null/-empty value be used instead that causes the same behavior (if any in current WordPress)? 3. Should the null value be still required for that constant, then a new a new issue ticket should be created for WordPress core that fixes the PHP warning (see the discourse discussion). |
@codepuncher: Can you comment out/remove the non-multisite part and deploy a multisite-site again? To exclude the possibility that this may be used (or is it listed as "skipped")? A PHP warning |
Gave that a go and had the same problem. However, I did find that on this particular project Log```log TASK [deploy : Invoke 'wp core is-installed' command] ************************** System info: Ansible 2.12.6; Darwin Trellis version (per changelog): "Add built-in fail2ban filters" --------------------------------------------------- non-zero return code PHP Fatal error: Uncaught Roots\WPConfig\Exceptions\ConstantAlreadyDefinedException: Aborted trying to redefine constant 'MULTISITE'. `define('MULTISITE', ...)` has already been occurred elsewhere. in /www/activatelearningcolleges_710/public/releases/2022 0607133101/vendor/roots/wp-config/src/Config.php:106 Stack trace: #0 /www/activatelearningcolleges_710/public/releases/20220607133101/vendor/ro ots/wp-config/src/Config.php(26): Roots\WPConfig\Config::defined() #1 /www/activatelearningcolleges_710/public/releases/20220607133101/config/ap plication.php(124): Roots\WPConfig\Config::define() #2 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1277) : eval()'d code(7): require_once('/www/activatele...') #3 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1277): eval() #4 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1235): WP_CLI\Runner->load_wordpress() #5 phar:///usr/local/bin/wp/vendor/wp-cli/wp- cli/php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\Runner->start() #6 ph in /www/activatelearningcolleges_710/public/releases/20220607133101/ven dor/roots/wp-config/src/Config.php on line 106 fatal: [kinsta_staging]: FAILED! => {"changed": false, "cmd": ["wp", "core", "is-installed", "--skip-plugins", "--skip-themes", "--require=/www/activatelearningcolleges_710/public/shared/tmp_multisite_constants.php"], "delta": "0:00:00.155150", "end": "2022-06-07 13:32:43.897521", "failed_when_result": true, "rc": 255, "start": "2022-06-07 13:32:43.742371", "stderr_lines": ["PHP Fatal error: Uncaught Roots\\WPConfig\\Exceptions\\ConstantAlreadyDefinedException: Aborted trying to redefine constant 'MULTISITE'. `define('MULTISITE', ...)` has already been occurred elsewhere. in /www/activatelearningcolleges_710/public/releases/20220607133101/vendor/roots/wp-config/src/Config.php:106", "Stack trace:", "#0 /www/activatelearningcolleges_710/public/releases/20220607133101/vendor/roots/wp-config/src/Config.php(26): Roots\\WPConfig\\Config::defined()", "#1 /www/activatelearningcolleges_710/public/releases/20220607133101/config/application.php(124): Roots\\WPConfig\\Config::define()", "#2 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1277) : eval()'d code(7): require_once('/www/activatele...')", "#3 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1277): eval()", "#4 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1235): WP_CLI\\Runner->load_wordpress()", "#5 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\\Runner->start()", "#6 ph in /www/activatelearningcolleges_710/public/releases/20220607133101/vendor/roots/wp-config/src/Config.php on line 106"], "stdout": "", "stdout_lines": []} ```So, really, this is a separate issue from what I'm seeing. I've tried a few things such as wrapping the Removing the Any ideas at this point @strarsis? Want to make sure I report log this issue in the appropriate repository, too, so where would that sit best @swalkinshaw? Cheers |
So according to that PHP error the |
That's the only place that constant is defined is within |
@codepuncher: This may be the underlying reason: Recent Bedrock doesn't set the Edit: Except that maybe Trellis adds this for multisite sites, but I can't find any occurrences of this except for the temporary PHP file used by the multisite is installed check (Edit: which i... |
🤔 yeah I'm guessing that was manually added at some point? The WP constant is |
Yeah, default Bedrock does not include the Removing both constants from Not sure what is going on here at this point 🤔 |
@codepuncher: Good point. The Trellis docs state But that |
I personally don't know anything about MU, but that constant is used by WP so it's not completely wrong that the docs say it: https://github.com/WordPress/WordPress/blob/8a90b8691ff7a7cf511ec624c912aacb6dd2b30a/wp-admin/network.php#L28-L30 |
@swalkinshaw: Right, the constant should be set. But why hadn't there been a conflict with the Trellis multisite check until now, at least for @codepuncher. That PHP file that sets the constant had always been included, so this error should have occurred before for multisite sites, but apparently it hadn't, that's what currently totally baffles me. @codepuncher: Do deploys of WordPress pre |
@swalkinshaw, @codepuncher: When I comment out the constants in
The commit that added this code explains this as the reason: 51f7b72 So when the multisite site already was set-up, commenting out these PHP constants fixes the PHP constants already defined exceptions - but for non-set-up multisites sites deployments will fail as shown above. So these PHP constants are only needed for non-set-up multisite sites. To completely fix this issue IMHO the multisite constants set in the Bedrock site config needs to be set as optional constants - the Before: /* Multisite */
Config::define('WP_ALLOW_MULTISITE', true);
Config::define('MULTISITE', true);
Config::define('SUBDOMAIN_INSTALL', false); // Set to true if using subdomains
Config::define('DOMAIN_CURRENT_SITE', env('DOMAIN_CURRENT_SITE'));
Config::define('PATH_CURRENT_SITE', env('PATH_CURRENT_SITE') ?: '/');
Config::define('SITE_ID_CURRENT_SITE', env('SITE_ID_CURRENT_SITE') ?: 1);
Config::define('BLOG_ID_CURRENT_SITE', env('BLOG_ID_CURRENT_SITE') ?: 1); After: /* Multisite */
Config::defineIfNotDefined('WP_ALLOW_MULTISITE', true);
Config::defineIfNotDefined('MULTISITE', true);
Config::defineIfNotDefined('SUBDOMAIN_INSTALL', false); // Set to true if using subdomains
Config::defineIfNotDefined('DOMAIN_CURRENT_SITE', env('DOMAIN_CURRENT_SITE'));
Config::defineIfNotDefined('PATH_CURRENT_SITE', env('PATH_CURRENT_SITE') ?: '/');
Config::defineIfNotDefined('SITE_ID_CURRENT_SITE', env('SITE_ID_CURRENT_SITE') ?: 1);
Config::defineIfNotDefined('BLOG_ID_CURRENT_SITE', env('BLOG_ID_CURRENT_SITE') ?: 1); Constants defined by this proposed |
Can you explain exactly what this state is and how you did it? I was testing last night as well and couldn't reproduce that database error. My process was:
|
When I manually go into the release directory of that site (no |
So I wonder if this only happens when going from non-multisite -> multisite? That would seem very rare though. If so, should Trellis even care to support that edge case? |
@swalkinshaw: From my description this was a bit confusing, sorry for that. But I deployed a non-multisite site and then a completely separate multisite site. To be sure I redo these steps with a clean system to verify. |
@swalkinshaw: Alright, I prepared something that can reproduce the issue and also demonstrate what and where exactly things go wrong:
The
This should demonstrate that...
Possible solutions that I can come up with:
|
❤️ 👏 thanks @strarsis. I'll try to replicate on a local VM |
Thinking out loud a bit... another way of identifying the "WP not installed" case is the initial or first deploy. For remote servers, the workflow is meant to be:
So I can think of two options that avoid this problem entirely 🤞 Detect initial deployWe could check if there's any existing deploys/releases in the This wouldn't support an edge case of someone doing multiple deploys before installing but maybe that's fine. Don't support multisite config on initial deployMaybe we could enforce this programmatically, but we could also document that Trellis requires the following workflow for multisite installs:
I'm not 100% this would work though and haven't tested it yet. |
@swalkinshaw: Yes, this is a kind of bootstrapping issue IMHO. WordPress has to be installed in order to be either initially set up or further configured by Trellis (e.g. installing languages). Trellis installs WordPress during deployment (using
The
However, when Trellis deploys a site for the first time - multisite or not - only the files are copied by How is Why is the |
It's not. See:
Correct, Trellis does nothing to the database if WP is not installed.
It's used to:
All the tasks related to those would fail if we tried to run them for non-installed sites.
I don't think I agree since deploys are mostly getting code on the server,
Yeah it lets us know we can run WP site commands basically since the database exists. |
@swalkinshaw: Thank you for this great and detailed explanation! Interestingly it also exits with The Ansible check condition requires an empty Modified - name: WordPress Installed (multisite)?
block:
- name: Set variables used in "WordPress Installed (multisite)?" check
set_fact:
multisite_non_setup_db_error: "WordPress database error Table '{{ site_env.db_name }}.wp_blogs' doesn't exist"
- name: "Invoke 'wp core is-installed' command"
command: wp core is-installed --skip-plugins --skip-themes
args:
chdir: "{{ deploy_helper.new_release_path }}"
register: wp_installed_multisite
changed_when: false
failed_when: (wp_installed_multisite.stderr | length > 0 and wp_installed_multisite.stderr is not match(multisite_non_setup_db_error)) or wp_installed_multisite.rc > 1
- name: Set "WordPress installed?" result variable (from multisite)
set_fact:
wp_installed: "{{ wp_installed_multisite }}"
when:
- project.multisite.enabled | default(false) After the now successful deploy, there may still be some redirection hiccups for the frontend URLs (redirection loop stuff) as the site wasn't fully set up now, which is to be expected, that site is deployed, but its configuration/content isn't prepared yet.
The database error occurs as a PHP warning at its initial run, but doesn't cause any issues, the setup will complete and everything is fine. |
I hadn't really thought about just catching that specific db error, but it makes sense. It has the minor downside of being tied to a specific error string, but not sure we can avoid that. And it's definitely simpler overall; getting rid of the tmp constants is a big win 👍 Want to do a PR with it? Thanks for all the help with this 🎉 |
@swalkinshaw: PR: #1391 |
Just seeing all of this now. Y'all are legends. Nice work. 💙 |
If the ping is okay, I still seem to be having an issue with a recently rebased Trellis site. |
@MikeiLL: See this response post: https://discourse.roots.io/t/wordpress-6-0-update-deploy-failed/23225/29?u=strarsis |
Should " |
Duh! FastCGI Process Manager. Lost in the weeds, was I. |
(Variant C) This PR splits the
WordPress installed?
check into two conditional ones,one for non-multisites and one for multisites.
The temporary PHP file for setting some constants to
null
/false
is then only created/ensured for multisites.The
wp core is-installed
invocation also is then different for non-multisites and multisites,and the PHP warning for
strpos()
empty needle for anull
WPMU_PLUGIN_DIR
constant (used in the multisite check)is ignored (as in the variant B PR #1387).
Edit: Fixes #1385.
Variant A PR: #1386(Edit: Closed now in favor of this one)Variant B PR: #1387(Edit: Closed now in favor of this one)Variant C PR: (This PR)
@swalkinshaw: IMHO this approach is the best one as it simplifies the check for non-multisite (single) sites,
while allowing the PHP warning for multisites sites.