Skip to content
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-cli v0.21.1 handles globals differently than WordPress #2289

Closed
rangerer opened this issue Dec 10, 2015 · 18 comments
Closed

wp-cli v0.21.1 handles globals differently than WordPress #2289

rangerer opened this issue Dec 10, 2015 · 18 comments

Comments

@rangerer
Copy link

We updated our multisite install to WordPress 4.4 and updated wp-cli to 0.21.1 along the way.

Since then the speed of wp-cli command decreased noticeably and plugin activate/deactivate seemed to work in wp-cli, but didn't work for WordPress.

We downgraded to wp-cli 0.20.4 and everything is working nicely again. However I need to run wp plugin deactivate (with version 0.21.1) again for all sites in order to get the plugin status in sync with WordPress again, before I can use version 0.20.4 again.

@danielbachhuber
Copy link
Member

Since then the speed of wp-cli command decreased noticeably

Can you use time to provide before and after benchmarks?

time wp option get home

plugin activate/deactivate seemed to work in wp-cli, but didn't work for WordPress.

WP-CLI doesn't load when you're using the WordPress admin, so WP-CLI has no possibility of affecting the WordPress admin experience.

@rangerer
Copy link
Author

Hi Daniel,

I just asked my coworker to supply some timing information (we do log wp-cli calls and their timing).

The major issue however is that plugin activation/deactivation was totally out of sync with WordPress and even broke the WordPress ability to manage plugin status itself.

@danielbachhuber
Copy link
Member

The major issue however is that plugin activation/deactivation was totally out of sync with WordPress and even broke the WordPress ability to manage plugin status itself.

This sounds like a problem with something else in your stack, stale cache for instance, not WP-CLI.

@rangerer
Copy link
Author

This sounds like a problem with something else in your stack, stale cache for instance, not WP-CLI.

Just what I thought, but I even disabled the cache completely - I also tried running it against different versions of WordPress. I will try to dig deeper into the issue when I have time - its reproducible for me, so I should be able to gather more insight.

@rangerer
Copy link
Author

Long story short I tracked down the problem to different globals handling ...

We set $memcached_servers in our wp-config.php and while this should be in the global scope (and it definitely is for WordPress and v0.20.4) and is not in the global scope for v0.21.1.

Of course the fix is as easy as using $GLOBALS['memcached_servers'] but you might want to consider rewriting the globals handling for this case, as setting a global in wp-config.php is quite common practice.

@rangerer rangerer changed the title Site Plugin activation/deactivation not working wp-cli v0.21.1 handles globals differently than WordPress Dec 11, 2015
@danielbachhuber
Copy link
Member

Globals need to be explicitly globalized, now that WP-CLI loads WordPress inside of a function #2089

@rangerer
Copy link
Author

Globals need to be explicitly globalized, now that WP-CLI loads WordPress inside of a function

No problem for me, but others might run into similar issues ...

@teajaymars
Copy link

This is a huge problem IMO, because lots of theme designers and plugin authors will have broken this rule without any clue of the implications. We use 30+ third-party plugins in our stack, including major plugins like https://en-gb.wordpress.org/plugins/redirection/ (400,000+ installs globally). WP CLI 0.21.1 will crash on load.

(It also took 2+ hours to work out why WordPress loads OK in the browser but not at all on the CLI. I count this as a very obscure bug...).

Obviously we can't afford to fix all third-party plugins, nor insure against future plugins breaking our stack, so we are now forced to downgrade WP CLI.

@danielbachhuber
Copy link
Member

This is a huge problem IMO, because lots of theme designers and plugin authors will have broken this rule without any clue of the implications.

I encourage you to open support tickets with them to produce fixes as needed. Given the sweeping nature of the change, and that only a few localized issues have been reported so far, I consider it a problem easily remedied by plugins / themes, and not something needing to be fixed with WP-CLI.

We use 30+ third-party plugins in our stack, including major plugins like https://en-gb.wordpress.org/plugins/redirection/ (400,000+ installs globally). WP CLI 0.21.1 will crash on load.

Have you debugged which plugin is actually causing the failure?

You can use --skip-plugins or --skip-themes to skip the loading of specific plugins or themes when using WP-CLI.

so we are now forced to downgrade WP CLI.

Please keep in mind there are unlikely to be further v0.20.x releases in the future. I'd encourage you to proactively solve the problem now, lest it becomes a larger problem for you in the future.

@danielbachhuber
Copy link
Member

We set $memcached_servers in our wp-config.php and while this should be in the global scope (and it definitely is for WordPress and v0.20.4) and is not in the global scope for v0.21.1.

@rangerer FYI - this will be fixed magically in v0.22.0, see #2318

@teajaymars
Copy link

Yes, I've specified which plugin is causing the current crash -- it's WP Redirection, which is not a niche or unpopular plugin.

I think the problem is worse than you're willing to admit: If you're running a business with WordPress in it (we maintain a growing number of WP sites in the .gov TLD) every code commit and every plugin activation must be checked against this additional constraint. We're going to have to build another test harness, because this is the kind of bug that sneaks in and then becomes apparent one day when a cron job mysteriously crashes in production.

@teajaymars
Copy link

I'd encourage you to proactively solve the problem now, lest it becomes a larger problem for you in the future.

This is important: Plugin/theme authors are not going to stop using global variables because of this. If we clean up our current stack the problem does not go away; we will always have to watch for this, and every new plugin needs to be double-checked before deployment. Customers won't want to hear that their custom code can't be deployed "because it's not compatible with one of our back-office tools", and we can't afford the additional technical burden of having to rewrite it for them.

I'm grateful to WP CLI for saving me many hours of work, and I understand where you are coming from, but you must see how much friction this change will cause. Putting my management hat on: We are more likely to throw away this one tool than to add maintenance burden to the other thirty tools.

@danielbachhuber
Copy link
Member

We're going to have to build another test harness, because this is the kind of bug that sneaks in and then becomes apparent one day when a cron job mysteriously crashes in production.

Why throw the baby out with the bathwater? Why not just be proactive about fixing the problem?

It appears Redirection largely uses Singletons, so I'm not sure how the WP-CLI change causes the problem you're referring to.

For that matter, can you share the problem you've run into, with steps on how to reproduce? Feel free to open it in the Redirection repo, and I'll follow along https://github.com/johngodley/redirection

This is important: Plugin/theme authors are not going to stop using global variables because of this.

That's fine. They just need to explicitly globalize the variables. It's bad practice to use global variables, and it's especially bad practice not to explicitly globalize them.

Plugins and themes can do lots of things to break WP-CLI. It's impossible to address all of them.

Customers won't want to hear that their custom code can't be deployed "because it's not compatible with one of our back-office tools", and we can't afford the additional technical burden of having to rewrite it for them.

Frankly, this conversation is mostly moot because you can just skip custom plugins or themes with the --skip-plugins and --skip-themes arguments. I'm not sure why you need to load Redirection in the WP-CLI context in the first place.

@teajaymars
Copy link

You are correct about --skip-plugins, and to be honest it does solve the problem for us, as long as we don't write any CLI Extensions inside a plugin (spoiler alert: We totally do, but will rewrite them...)

@danielbachhuber
Copy link
Member

You can also skip specific plugins with --skip-plugins=<plugin-slug>

@teajaymars
Copy link

Thanks. Although that won't help when connecting to an arbitrary WordPress instance which might have yet-unseen plugins installed by a client or technician which could contain undeclared globals. It might be possible to do something crazy like: wp --skip-plugins plugin list | xargs ... to grab the list of plugins and disable everything except recognised whitelisted plugins.

@rangerer
Copy link
Author

@rangerer FYI - this will be fixed magically in v0.22.0, see #2318

Good stuff - didn't know about get_defined_vars() so far.

I tend to agree with @danielbachhuber that defining globals explicitely is the more elegant (and maintainable) solution - especially in wp-config.php, where I have control over it - just a suggestion for future readers of this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants