fix is_wp_version_compatible() to accommodate incorrect format x.x.0

[afragen]

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[aaronjorbin]

Great start. I think this might need a test to ensure that the notice is properly raised. I also left a couple of comments on the specific implementation.

[aaronjorbin]

This should use wp_trigger_error

[afragen]

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.

[aaronjorbin]

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 is_wp_version_compatible, 6.0 is assumed."

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[afragen]

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?

[aaronjorbin]

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]

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?

[aaronjorbin]

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

[costdev]

These latest changes provide a false sense of security, as does the test in this Core changeset.

For example, if you comment out all datasets except those that should throw an exception, and change the final assertion to $this->assertSame( $expected . 'should not pass', $result );, the two datasets will still pass. This is because an exception was correctly detected, but the test also ended as soon as this occurred. This issue also exists with the test in the Core changeset linked above.

Instead, let's consider this approach:

1. Add a new property to the test class:

/**
 * A notice expected during the tests.
 *
 * @var string
 */
private $expected_notice = '';

2. Add a tear_down() method that resets the property:

/**
 * Resets the `expected_notice` property after each test runs.
 */
public function tear_down() {
	$this->expected_notice = '';
	parent::tear_down();
}

3. Adjust the test so that instead of throwing an exception, we note the notice, restore the previous handler, and check that it matches the expected type and string:

public function test_is_wp_version_compatible( $required, $expected, $expect_notice = false ) {
	if ( $expect_notice ) {
		set_error_handler(
			function ( $errno, $errstr ) {
				$this->expected_notice = $errno . ' ' . $errstr;
			},
			E_ALL
		);
	}

	$result = is_wp_version_compatible( $required );

	if ( $expect_notice ) {
		restore_error_handler();
		$this->assertStringStartsWith(
			E_USER_NOTICE . " is_wp_version_compatible(): `$required` Not a valid WordPress version string.",
			$this->expected_notice,
			'The expected notice was not thrown.'
		);
	}

	$this->assertSame( $expected, $result );
}

Now, if we change the final assertion to $this->assertSame( $expected . 'should not pass', $result );, the two datasets will correctly fail.

Run a few checks locally to see the issue with the existing test, and how the changes above resolve this.

[costdev]

Looks like the discussion has moved away from having a notice. The above can be disregarded in that case. 🙂

[costdev]

Un-resolving this as a decision on the notice hasn't finalized.

@aaronjorbin FYI: Take a look at the first comment in this thread regarding an issue with the changeset you referenced and previous commits to this PR that were influenced by it, and an alternative without this issue.

[peterwilsoncc]

Is it worth adding a test to ensure the version is converted correctly? It might require some code shuffling.

[costdev]

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.

[afragen]

I added this test to ensure a .0 in the middle of a version number wouldn’t be changed.

[afragen]

I will add some new tests for that. I'll have it fixed by tomorrow.

[azaozz]

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.

Edit: actually this should probably be strlen( $required ) > 4 to catch WP versions up to 99.0 (that would still work properly). Seems reasonable to update this code when WP gets to 100.0 :)

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @[email protected].

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

Core Committers: Use this line as a base for the props when committing in SVN:

Props afragen, peterwilsoncc, jorbin, costdev, azaozz, swissspidy.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[azaozz]

See #5677 (comment).

[peterwilsoncc]

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

[azaozz]

$required is developer input that can't be trusted to be entirely correct...

Right. Thinking it looks better now, and perhaps a very tiny bit faster (afaik PHP needs some time to "initialize" the external lib every time a regexp function is used) :)
Seems this is good to go if there isn't anything else.

[peterwilsoncc]

Merged r57707 / 2ba8f94