$cp_version and WP-CLI

General Discussion Coding Help
1

I was looking at ZP Remove Customizer (tagging @anon95694377 here) that deactivates itself when i run
wp plugin update --all.

Really this is not a bug in the plugin, but it’s all about compatibility with WP-CLI and so i wanted to share some consideration:

  • Can be a good idea to collect those issues and cope with strange behaviours of WP-CLI (really at the moment it’s 99% compatible)?
  • Can we fix some of them in the core?
  • Can we say bye bye to WP-CLI?

Now… about the issue… the code is this:

			if ($this->is_classicpress()) {
				if (version_compare(classicpress_version(), '1.1', '<')) { 
					$this->autodeactivate('ClassicPress 1.1.x'); 
				}
			}

The problem is that $cp_version is not defined as a global, so classicpress_version() returns null when in command line. Examples:

> wp eval 'global $cp_version; var_dump($cp_version);'
NULL
> wp eval 'global $wp_version; var_dump($wp_version);'
string(6) "4.9.13"

If i add this to version.php it works.

$GLOBALS['cp_version'] = $cp_version;

> wp eval 'global $cp_version; var_dump($cp_version);'
string(5) "1.1.2"

Or this works too (in the plugin):

			if ($this->is_classicpress()) {
				if (version_compare(classicpress_version(), '1.1', '<') && !defined('WP_CLI')) { 
					$this->autodeactivate('ClassicPress 1.1.x'); 
				}
			}

Any consideration about it?
Simone.

2 Likes
2

Hi Simone, thanks for posting about this. I’m not a big CLI user so wasn’t aware of the issue.

If there is agreement that the change to version.php can be adopted into ClassicPress core, then from my point of view as a plugin developer, this would be the ideal solution.

And it certainly seems logical that having added the classicpress_verison function to core, this should be made to work from CLI as well as in PHP files.

However, if this can’t be done for some time, then I don’t mind amending my version checking method in my plugins.

I’ll keep watching this thread.

3 Likes
3

Maybe the fix can be done in a dedicated file or class.

I’m now testing this code that fixes the wrong behaviour of wp core check-updates.

If I’m not wrong there are really few things incompatible with WP-CLI, so having them fixed into ClassicPress core will be nice.

1 Like
4

During the normal ClassicPress loading process, $cp_version is defined as a global. My guess is that WP-CLI is loading things differently.

This would break the core upgrade process, since the version.php file from the new package is loaded inside a function (in which case we don’t want $cp_version to be re-set globally).

In order to fix this properly, we need to know a bit more about the WP-CLI loading process and how it ends up including version.php.

More generally:

  • Can be a good idea to collect those issues and cope with strange behaviours of WP-CLI (really at the moment it’s 99% compatible)? Yes, as a good first step.
  • Can we fix some of them in the core? Yes, definitely.
  • Can we say bye bye to WP-CLI? I would not recommend this right now, because I expect WP-CLI to continue to be 99% compatible with ClassicPress for a while.
1 Like
5

Ok, seems to be the same problem because WP-CLI is loading version.php in this way here.

/*
 * These can't be directly globalized in version.php. When updating,
 * we're including version.php from another installation and don't want
 * these values to be overridden if already set.
 */
global $wp_version, $wp_db_version, $tinymce_version, $required_php_version, $required_mysql_version, $wp_local_package;
require ABSPATH . WPINC . '/version.php';

I’ve tried this (within a plugin) and seems to work, but it’s really over my skills to understand if this can break something.
Edit: I’ve tried to upgrade and it works both from backend and CLI, but i’m not sure of whatelse can break.

if (defined('WP_CLI') && WP_CLI) {
	WP_CLI::add_hook('before_wp_config_load', 'cp_version_globalize');
}

function cp_version_globalize() {
	global $cp_version;
	require ABSPATH . WPINC . '/version.php';
}
1 Like
6

It’s a good idea to be extra careful with load order changes (things like which functions get called first, or whether variables are defined in global scope or function scope). It can be hard to know what will break in these cases, but there is such a huge amount of code out there that interacts with our ecosystem that there is almost always something. And since we are a smaller ecosystem than WordPress, we may not notice these problems for a while. Then once that happens, we have code in ClassicPress plugins that depends on the way our system works, but site owners are also trying to use code from WordPress plugins that depends on the way WP works, and it could turn into a pretty big mess.

The only way to avoid this situation 100% is to understand 100% of the code that could be running in these contexts, which is basically impossible. A good general guideline to follow is to make changes as narrowly as possible (make sure the only thing that changes is the specific thing you’re trying to fix, in the specific situation where it’s broken).

So enough general philosophy for now…

I think this is an excellent thought process to fix this issue. Doing something like this should make the $cp_version variable global, but only when WP-CLI is loading, and only once. I think this should fix the issue as narrowly as possible without introducing other side effects, for example by making the wrong $cp_version value global again during a core upgrade.

My only question would be where to put this code inside ClassicPress core so that it makes sense, and works. I haven’t tried it yet, but at this moment I don’t understand how we can insert a hook into WP-CLI before the core code even loads.

Here’s another possible approach that should do the same thing just using some new code inside version.php:

if ( defined( 'WP_CLI' ) && WP_CLI && ! defined( 'WP_CLI_CP_VERSION_LOADED' ) ) {
    // ClassicPress is being loaded using WP-CLI. Make sure we set the
    // `$cp_version` variable globally so that other code can see it.
    $GLOBALS['cp_version'] = $cp_version;
    // Make sure we do this only once (to avoid potential unintended effects
    // when updating ClassicPress using WP-CLI).
    define( 'WP_CLI_CP_VERSION_LOADED', true );
}

The WP_CLI_CP_VERSION_LOADED bit is intended to make the fix as narrow as possible, to avoid potential issues with the wrong $cp_version variable being set globally during an upgrade, or during any other code which loads version.php by itself.

1 Like
7

Ok, I gave a try at both ways, and they both work.
Work = no explosions, no fire, no warnings, core updates tested using both CLI and backend.

Changing version.php

Your code works, I’ve cut it out a bit and this works too:

if ( defined( 'WP_CLI' ) && WP_CLI && ! is_set( $GLOBALS['cp_version'] ) ) {
    // ClassicPress is being loaded using WP-CLI. Make sure we set the
    // `$cp_version` variable globally so that other code can see it.
    $GLOBALS['cp_version'] = $cp_version;
}

I think that if the global is set we are sure not to do it multiple times.
Or is it a wrong assumption?

Hooking in WP-CLI

I’ve added a new file (wp-includes/fix-cli.php).
Here we could put any fixes.
Then included this from wp-settings.php .

    define( 'WPINC', 'wp-includes' );

    require( ABSPATH . WP-INC . '/fix-cli.php' );

Edit: putting code here works but don’t know if it makes sense.

Here I’ve this code:

<?php 
// Fix wp-cli to have $cp_version globalized in the same way as $wp_version.
if (defined('WP_CLI') && WP_CLI) {
	WP_CLI::add_hook('before_wp_config_load', 'cp_globalize');
}

function cp_globalize() {
	if (isset($GLOBALS['cp_version'])) {
        //  Exit if $cp_version is already global.
		exit;
	}
	global $cp_version;
	require ABSPATH.WPINC.'/version.php';
}

In this file maybe we can put the fix for wp core check-update…
Hooking to WP-CLI can limit damages as it can only break WP-CLI.

1 Like
8

I think this is right, good idea. However, it looks like there is an even simpler way. Reference: wp-cli/php/WP_CLI/Runner.php at d2a002214df900bba7303310f42c68b996d873ab · wp-cli/wp-cli · GitHub

Since our $wp_version will always be greater than 4.6, WP-CLI won’t use its wp-settings-cli.php file. The normal wp-settings.php file from ClassicPress will be loaded instead. I missed this the first time I looked at this code.

So I think all we need to do is add global $cp_version here: ClassicPress/src/wp-settings.php at f765dbfa00cf74fd51cae45ffb13cd2536417e4b · ClassicPress/ClassicPress · GitHub

Let me know if you can test that change, I think it should be the last iteration of this fix :grimacing:

I agree with this approach for future changes. We might call it wp-includes/cp-compat.php.

1 Like
9

Tested, it works!
Great!
As I can understand your second link was double pasted… you meant ClassicPress/src/wp-settings.php at f765dbfa00cf74fd51cae45ffb13cd2536417e4b · ClassicPress/ClassicPress · GitHub

2 Likes
10

Yes, that’s right. Edited, thanks!

1 Like
11

This topic was automatically closed 2 days after the last reply. New replies are no longer allowed.