-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix is_wp_version_compatible() to accommodate incorrect format x.x.0 #5677
Changes from 12 commits
f70cd67
a012763
e41c513
3e6a7bd
603dcf9
e08ac43
4eb9190
54e7518
3cad191
ea216d5
93b0126
fc88bb5
7098415
1d731cb
d54c5d6
e45ddce
68b7f48
3781e42
85845cd
bcbff68
1f236e3
c5d38ce
014d45d
27a5a64
c7d07f0
0482621
4e7367b
3ff62db
e4f52ae
e1429fe
bd5a45d
0cfcdeb
bccd14f
5549256
fdda8f0
4dcf147
b6a1f8f
4edb63d
0d27643
d11dbe3
c5b3a67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8716,6 +8716,16 @@ function is_wp_version_compatible( $required ) { | |
// Strip off any -alpha, -RC, -beta, -src suffixes. | ||
list( $version ) = explode( '-', $wp_version ); | ||
|
||
if ( is_string( $required ) && substr_count( $required, '.' ) > 1 && str_ends_with( $required, '.0' ) ) { | ||
error_log( | ||
__FUNCTION__ . '(): ' . | ||
/* translators: s: version string sent to function */ | ||
sprintf( __( '`%s` Not a valid WordPress version string.' ), $required ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to present both what was passed in and what the result was. What do you think of this instead? "Invalid version 6.0.0 passed to |
||
); | ||
|
||
$required = rtrim( $required, '.0' ); | ||
} | ||
|
||
return empty( $required ) || version_compare( $version, $required, '>=' ); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,32 @@ public function data_is_wp_version_compatible() { | |
'required' => array(), | ||
'expected' => true, | ||
), | ||
|
||
// Improper WP version. | ||
'improper trailing x.x.0' => array( | ||
'required' => '5.2.0', | ||
'expected' => true, | ||
), | ||
'correct version' => array( | ||
'required' => '5.2', | ||
'expected' => true, | ||
), | ||
'incorrect trailing x.0.0' => array( | ||
'required' => '5.0.0', | ||
'expected' => true, | ||
), | ||
'correct version x.0' => array( | ||
'required' => '5.0', | ||
'expected' => true, | ||
), | ||
'correct version x.0.1' => array( | ||
'required' => '5.0.1', | ||
'expected' => true, | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth adding a test to ensure the version is converted correctly? It might require some code shuffling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Provided there are tests covering all the bases, testing the output with each input should be enough, and the respective lines can be checked for coverage in a coverage report. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this test to ensure a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add some new tests for that. I'll have it fixed by tomorrow. |
||
'correct version x.1.1' => array( | ||
'required' => '5.1.1', | ||
'expected' => true, | ||
), | ||
); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use
wp_trigger_error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was initially having issues with tests not passing due to the
wp_trigger_error()
message being returned but found a solution by removing the error handler before the test and resetting afterwards.