Make WordPress Core

Opened 14 months ago

Closed 9 months ago

Last modified 9 months ago

#59448 closed defect (bug) (fixed)

is_wp_version_compatible works unreliably

Reported by: kkmuffme's profile kkmuffme Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.5 Priority: normal
Severity: minor Version: 5.2
Component: Upgrade/Install Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

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

Attachments (1)

59448.patch (1.5 KB) - added by sessioncookiemonster 12 months ago.
Patch for this

Download all attachments as: .zip

Change History (38)

#1 @afragen
14 months ago

  • Component changed from General to Upgrade/Install
  • Severity changed from blocker to minor

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.

#2 @kkmuffme
13 months ago

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.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


12 months ago

#4 @jorbin
12 months ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 5.2

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
12 months ago

Patch for this

#5 @sessioncookiemonster
12 months ago

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

#6 @jorbin
12 months ago

  • Keywords 2nd-opinion added

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.

#7 @afragen
12 months ago

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.

#8 @afragen
12 months ago

Last edited 12 months ago by afragen (previous) (diff)

This ticket was mentioned in PR #5677 on WordPress/wordpress-develop by @afragen.


12 months ago
#9

  • Keywords has-patch has-unit-tests added; needs-patch removed

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

#10 @afragen
12 months ago

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.

This ticket was mentioned in Slack in #core by afragen. View the logs.


11 months ago

#12 follow-up: @kkmuffme
11 months ago

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)

#13 @afragen
11 months ago

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.

#14 in reply to: ↑ 12 @afragen
11 months ago

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.

This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.


10 months ago

@afragen commented on PR #5677:


10 months ago
#16

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?

@jorbin commented on PR #5677:


10 months ago
#17

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.

@afragen commented on PR #5677:


10 months ago
#18

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?

@jorbin commented on PR #5677:


10 months ago
#19

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

#20 follow-up: @peterwilsoncc
10 months ago

@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.

#21 in reply to: ↑ 20 @afragen
10 months ago

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.

#22 @afragen
10 months ago

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

#23 @peterwilsoncc
9 months ago

@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.

#24 follow-up: @jorbin
9 months ago

@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.

#25 @afragen
9 months ago

@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, 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.

Version 0, edited 9 months ago by afragen (next)

#26 @costdev
9 months ago

@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.

#27 in reply to: ↑ 24 @peterwilsoncc
9 months ago

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.

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.

#28 @afragen
9 months ago

I believe the PR is ready for commit.

This ticket was mentioned in Slack in #core by afragen. View the logs.


9 months ago

#30 @afragen
9 months ago

  • Keywords good-first-bug removed
  • Owner set to afragen
  • Status changed from new to accepted

@azaozz commented on PR #5677:


9 months ago
#32

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.

@peterwilsoncc commented on PR #5677:


9 months ago
#33

@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.

#34 @peterwilsoncc
9 months ago

  • Keywords 2nd-opinion removed

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?

#35 @swissspidy
9 months ago

  • Keywords commit added
  • Milestone changed from Future Release to 6.5
  • Owner changed from afragen to peterwilsoncc
  • Status changed from accepted to assigned

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

#36 @peterwilsoncc
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.