is_wp_version_compatible works unreliably

[kkmuffme]

on all major WP version releases is_wp_version_compatible works unreliably.
(the root cause is https://core.trac.wordpress.org/ticket/59403, but the function needs to be fixed too)

e.g. on WP 6.3, if you pass in '6.3.0' you would get back false, which is not what you'd expect

[afragen]

But there is no WP version 6.3.0. The version number is 6.3.

WordPress doesn’t conform to precise semantic versioning and has never used the x.x.0 format.

[kkmuffme]

Yes, that's why the other issue was opened to correct that, but that is something else.

The issue here is that 6.3 and 6.3.0 will return different results, which isn't what anybody would usually expect.

[jorbin]

is_wp_version_compatible was introduced in 5.2, so adjusting the version.

As @afragen noted, 6.3.0 isn't a valid release. So the question becomes what should happen if you pass in an invalid version. I think what's best here is to assume that someone means 6.3 in this example.

I think the path forward for this is to maybe throw a doing_it_wrong or wp_trigger_error if you pass in x.y.0 and strip off the .0 so that version_compare works correctly.

The patch for this should also include tests

[sessioncookiemonster]

Patch for this

[sessioncookiemonster]

I threw a _doing_it_wrong, like @jorbin suggested, set the version to X.X, since I don't know when this might be added

[jorbin]

Thanks @sessioncookiemonster for the patch. A few notes:

• Please make sure to use the WordPress coding standards. PHPCS should be able to help with this.
• The string needs to be translatable. I also think it might be good to make this error as descriptive as possible, maybe something like Invalid Version string passed to is_wp_version_compatible. $version passed but $valid_request assumed.

Also, @costdev pointed out in slack that wp_trigger_error might be better to use. I'm not sure yet but would love to hear some other folks chime in.

[afragen]

Just a thought but what about using str_ends_with() and then rtrim() ?

Result would then need to ensure proper x.0 version for replacement.

Just thinking it might be more clear what we’re trying to do than with regex.

[afragen]

Quick proof ​https://3v4l.org/p0qa2

Some users improperly test for releases of the format x.x.0. Unfortunately there is never a WordPress release with this version number and version_compare( '5.2', '5.2.0', '>=' ) is false.

Test value sent to is_wp_version_compatible( $required ) to ensure a trailing zero (x.x.0) is fixed to be (x.x) and a wp_trigger_error() is sent.

This will correctly test the improper version x.x.0 as x.x.

Trac ticket: https://core.trac.wordpress.org/ticket/59448

[afragen]

Using error_log() as wp_trigger_error() results in exiting the function and therefore cannot correct the value x.0.0 to x.0 and have the script continue.

[kkmuffme]

str_ends_with is PHP 8, but WP supports PHP 7, this won't work.

Instead of generating a useless error, this function should gracefully handle this and treat 6.3.0 and 6.3 as the same thing.
This will also make it easier if/once WP moves to this notation in the first place.
(there's an open meta ticket for that since a while but it wasn't high priority yet)

[afragen]

As of WordPress 5.9 there is a shim included for str_ends_with(), str_starts_with(), and str_contains(). ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/compat.php

It will absolutely work, assuming this fix isn't back ported beyond WordPresss 5.9. As it's not a security fix I doubt it will be.

[afragen]

Replying to kkmuffme:

Instead of generating a useless error, this function should gracefully handle this and treat 6.3.0 and 6.3 as the same thing.

My PR does treat 6.3.0 and 6.3 as equivalent and so the function will return true.

The error isn’t useless as it does point out that the user/dev is doing it wrong by using a version number that doesn’t technically exist.

Thanks @aaronjorbin, I believe I've fixed the issues you've raised.

I don't quite understand what you mean by ensuring the notice is properly raised?

Thanks, the code changes look great. Also, glad to hear this helped find another spot where our tests could be improved!

I don't quite understand what you mean by ensuring the notice is properly raised

I meant using something like what was implemented in https://core.trac.wordpress.org/changeset/57332 Essentially rather than silencing the error, setting an expect for it to be raised.

The problem with raising the error to an Exception is that the comparison will then fail and what we've intended is for the provided incorrect version be modified so it's now correct and the comparison continues -- or am I missing something?

Sorry I wasn't clear. I pushed some code to your branch since I thought showing the code would help.

By using expectException, the exception won't cause the tests to fail. I did move the actual call to is_wp_version_compatible to it's own line so that the error handler can be restored to it's previous state.

Let me know if this makes sense. It looks like I'll need to clean up some formatting as well, so I can try to add some comments if you think it would help

[peterwilsoncc]

@afragen Raising it here because I don't think it helps on the PR.

I'm wondering if WordPress should gracefully handle x.x.0 without logging a notice as the format is semi-official. While WordPress publicly refers to x.x as the version number, the ​official git mirrors tag with x.x.0 to avoid conflicts with branch names.

[afragen]

Replying to peterwilsoncc:

@afragen Raising it here because I don't think it helps on the PR.

I'm wondering if WordPress should gracefully handle x.x.0 without logging a notice as the format is semi-official. While WordPress publicly refers to x.x as the version number, the ​official git mirrors tag with x.x.0 to avoid conflicts with branch names.

Thanks @peterwilsoncc. Let me know if the decision is to remove the wp_trigger_error() notice as this would greatly simplify the code.

I’ll update accordingly.

[afragen]

@jorbin do you have thoughts on the silently correcting the value?

[peterwilsoncc]

@afragen @jorbin It occurred to me over the weekend that Core @since tags use the format x.x.0 (see example) so the most likely cause of developers using the incorrect format is via copy-paste from the functions they are checking.

Let's fix it silently as copying Core's format is reasonable.

[jorbin]

@peterwilsoncc It sounds to me like what should happen is that those function docs should be cleaned up.

Fixing this silently is essentially saying that there are two version numbers for any given version, the notice for developers should be included.

[afragen]

@jorbin the whole issue is that version_compare sees 5.0.0 as different that 5.0. Fundamentally I understand this but as WordPress doesn't follow semantic versioning, silently fixing the issue is much simpler.

The notice would only display for users with WP_DEBUG set to true and it makes writing the tests much more difficult.

I have updated the PR removing the wp_trigger_error(). If desired it can be added back.

[costdev]

@peterwilsoncc @jorbin I opened a PR against @afragen's which has now been merged.

This:

• adds the notice back to is_wp_version_compatible().
• separates the tests into test_is_wp_version_compatible() and test_is_wp_version_compatible_should_throw_notice() so that we're testing the function's behaviour, rather than testing a patch.
• handles the notice in the test by storing E_USER_NOTICEs for checking in the test, or throwing an Exception for non-E_USER_NOTICE.
  • This is abstracted to a private method to reduce noise in the test method.
• adds some extra datasets for the test checking for notices.

See ​Andy's PR for the latest iteration.

[peterwilsoncc]

Replying to jorbin:

Fixing this silently is essentially saying that there are two version numbers for any given version, the notice for developers should be included.

@jorbin I think you've identified the problem, in practice there are two version numbers used by the project for major releases.

• x.x.0 formatted
  • @since tags per the ​documentation standards - 14.5k instances
  • the above is reflected in the ​developer code reference
  • tags on the wordpress.org hosted git mirrors, both built and wordpress-develop
  • tags on the GitHub mirror of ​WordPress\wordpress-develop
• x.x formatted
  • tags on the wordpress.org hosted SVN repos, both built and wordpress-develop
  • tags on the GitHub mirror of ​WordPress\WordPress (built version)
  • publicity material

I think the x.x.0 formatted version numbers are used in the places most developers interact with WordPress. In practice, it's pretty much only committers that use the SVN repos and only when it comes time to commit.

[afragen]

I believe the PR is ready for commit.

[afragen]

Lots of great discussion in devchat, ​https://wordpress.slack.com/archives/C02RQBWTW/p1708549402218919

Thinking this code is far too complex for what it needs to accomplish. We only care to strip .0 from the end of $required, when it is longer than 3 characters, nothing more. So substr_count() and preg_replace() for a simple string that contains only digits and dots is perhaps "too much"?

Perhaps something simpler would also work:

if ( is_string( $required ) && strlen( $required ) > 3 && str_ends_with( $required, '.0' ) ) {
    // Strip trailing zero to properly match two digits (major) WP version numbers.
    $required = substr( $required, 0, -2 );
}

The strlen( $required ) > 3 is there to catch versions like 5.0, 6.0, etc. In all other cases the WP versions should not have a trailing zero.

@azaozz I've switched substr to remove the version number's trailing .0

$required is developer input that can't be trusted to be entirely correct (the entire point of this ticket ;) so I've kept string count.

[peterwilsoncc]

The ​linked pull request looks to be in good shape and ready for commit. @swissspidy @davidbaumwald are you happy if I move this to 6.5 for commit?

[swissspidy]

@peterwilsoncc Just left a tiny remark on the PR, otherwise looks good to me. Feel free to commit with that adjustment in place.

[peterwilsoncc]

In 57707:

Upgrade/Install: Normalize major versions in is_wp_version_compatible().

Modify is_wp_version_compatible() to return the expected result for major WordPress versions formatted as either x.x or x.x.0 (for example 6.5 and 6.5.0).

The WordPress project currently documents major version numbers in both formats leading to confusion for developers using the is_wp_version_compatible() function. As the PHP function version_compare() treats x.x and x.x.0 as different version numbers this leads to unexpected results in the WP function.

This change removes a trailing .0 from major version numbers to account for the WordPress project using the two formats interchangeably.

Props afragen, azaozz, costdev, joemcgill, jorbin, kkmuffme, sessioncookiemonster, swissspidy, wazeter.
Fixes #59448.

Merged r57707 / ​https://github.com/WordPress/wordpress-develop/commit/2ba8f942cb8e95bf93d19c12f9cd9bb9ae61b72e