#59448 closed defect (bug) (fixed)
is_wp_version_compatible works unreliably
Reported by: | kkmuffme | Owned by: | 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)
Change History (38)
#1
@
14 months ago
- Component changed from General to Upgrade/Install
- Severity changed from blocker to minor
#2
@
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
@
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
#5
@
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
@
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
@
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
@
12 months ago
Quick proof https://3v4l.org/p0qa2
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
@
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:
↓ 14
@
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
@
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
@
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
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?
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.
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?
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:
↓ 21
@
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
@
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 tox.x
as the version number, the official git mirrors tag withx.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.
#23
@
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:
↓ 27
@
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
@
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.
#26
@
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()
andtest_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_NOTICE
s 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.
- This is abstracted to a
- adds some extra datasets for the test checking for notices.
See Andy's PR for the latest iteration.
#27
in reply to:
↑ 24
@
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.
- 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.
This ticket was mentioned in Slack in #core by afragen. View the logs.
9 months ago
#30
@
9 months ago
- Keywords good-first-bug removed
- Owner set to afragen
- Status changed from new to accepted
#31
@
9 months ago
Lots of great discussion in devchat, https://wordpress.slack.com/archives/C02RQBWTW/p1708549402218919
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
@
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?
But there is no WP version
6.3.0
. The version number is6.3
.WordPress doesn’t conform to precise semantic versioning and has never used the
x.x.0
format.