Prevent activation of a plugin if its required PHP version is too high

[flixos90]

Note: This ticket is a subtask for the overarching #40934 ticket.

While the plans from #43986 and #43987 will ensure nobody can install or update plugins that require a PHP version higher than the version used, a third step should be to prevent plugin activations of said plugins. It is still possible to just upload plugin directories and then activate them from there. While the above tickets will cover the majority of cases, we should also account for the latter.

A difference between other work and this one would be that here, we need to read the required PHP version from the plugin readme file directly instead from the w.org plugins API.

[Luciano Croce]

This ticket is fantastic: i am fully in agreement with it, and at its introduction.

I have introduced this good practice in all my plugins for a long time''

[afragen]

I've been working on this and should have a patch ready soon. I'll need help with any unit tests.

[afragen]

Prevents plugin activation if either minimum WP or minimum PHP versions not met.

[afragen]

@SergeyBiryukov we can reuse the is_compatible_wp() and is_compatible_php() functions in several of the other Servehappy patches.

[afragen]

Allow for plugins not in Plugin Directory but containing plugin headers Requires WP and/or Requires PHP

[afragen]

Need to add Requires WP and Requires PHP as default plugin headers.

[afragen]

Allow for API response to error

[afragen]

renamed function and more complete docs

[afragen]

Improved error checking of API response

[afragen]

fixed docBlock error

[afragen]

A difference between other work and this one would be that here, we need to read the required PHP version from the plugin readme file directly instead from the w.org plugins API.

Before we can do this some sort of stripped down version of the dot org class-parser.php will need to be included with core.

Once added it should be simple to swap the internals of get_plugin_validation_data() to use the new local parser and parse the local readme.txt.

[afragen]

Reverse logic of validate_plugin_activation() to improve readability

[afragen]

fix version_compare so 5.1.10 is greater than 5.1.2 and true

[afragen]

We probably need to change milestone to 5.2

[afragen]

patch refreshed for 5.2 milestone

[afragen]

rename function to is_wp_compatible() and is_php_compatible() for readability

[afragen]

now based on new local readme parser

[afragen]

43992.12.diff​ contains a new(ish) readme parser for a local plugin readme.txt file. It is mostly a pared down version of the ​WordPress.org Plugin Readme Parser

No more calls to w.org plugins API.

[afragen]

improve sanitization and docblocks

[joyously]

I only looked at the last diff, but it is adding unnecessary code for reading the plugin file.
The headers are added to get_plugin_data, which is already reading the plugin file, so that function should be where the default value is returned if needed.

[afragen]

Replying to joyously:

I only looked at the last diff, but it is adding unnecessary code for reading the plugin file.
The headers are added to get_plugin_data, which is already reading the plugin file, so that function should be where the default value is returned if needed.

I admit I'm a little confused. In what part of the diff are you referring to and what do you think is unnecessary?

[joyously]

The WP_Readme_Header_Parser is unnecessary. The get_plugin_data function does that.

[afragen]

Replying to joyously:

The WP_Readme_Header_Parser is unnecessary. The get_plugin_data function does that.

Actually it doesn't do the same thing. get_plugin_data() parses and returns the headers in the main plugin file. The new class WP_Readme_Header_Parser will parse a local readme.txt file and return the headers.

Please refer to #10 above.

[joyously]

Comment 10 is talking about reading local versus using the API. get_plugin_data calls get_file_data which reads from the local file. The first parameter is the file to use, so you pass it the readme instead of the main plugin file. Still don't need your WP_Readme_Header_Parser.

[afragen]

Replying to joyously:

Comment 10 is talking about reading local versus using the API. get_plugin_data calls get_file_data which reads from the local file. The first parameter is the file to use, so you pass it the readme instead of the main plugin file. Still don't need your WP_Readme_Header_Parser.

Comment 10 references the last part of the ticket description. Obtaining the data from a local readme.txt file and not from the API. The API parses the uploaded readme.txt file.

get_file_data() reads from a plugin or theme main file, not from the readme.txt. get_file_data() references ​File Header in the Codex. get_file_data() will not correctly parse a readme.txt file.

Specifically from the Codex:

File Headers in readme.txt
Some plugins contain the readme.txt file which might contain look-a-like headers as well. Those files are  not handled by WordPress but by third-party applications. Because those applications can be quite popular, I note down here those tags from an example readme file:

Contributors: markjaquith, mdawaffe (this should be a list of wordpress.org userid's)
Donate link: http://example.com/
Tags: comments, spam
Requires at least: 2.0.2
Tested up to: 2.1
Stable tag: 4.3

As ticket #12260 suggests, the headers from readme.txt are used through remote WP.org API calls. This is a good example of how third party applications use has direct impact on wordpress core code usage.

[joyously]

get_file_data() will not correctly parse a readme.txt file.

I bet it will, and it would be better than adding a lot of extra code. It takes parameters for the file to use and the headers to look for. What's the big deal? The format is the same. Did you try it?

But I notice that this diff adds those headers to the defaults in get_plugin_data(), so are they just in the readme or in the plugin file also?

[afragen]

Replying to joyously:

get_file_data() will not correctly parse a readme.txt file.

I bet it will, and it would be better than adding a lot of extra code. It takes parameters for the file to use and the headers to look for. What's the big deal? The format is the same. Did you try it?

But I notice that this diff adds those headers to the defaults in get_plugin_data(), so are they just in the readme or in the plugin file also?

The overwhelming majority of plugins in dot org do not contain the added Requires WP and/or Requires PHP headers, but they all contain a readme.txt.

In fact, these were added to this patch as a method for plugins that are not hosted in dot org to be compliant and not necessarily require a valid readme.txt file.

As an example, a readme header of Requires or Requires at least, which are the excepted readme headers, will not parse correctly from get_plugin_data and will not return expected results.

Please submit a patch based on your idea. I'm happy to test it.

[afragen]

@aaroncampbell pinging for a sanity check as discussed at WordCamp Phoenix Contributor Day. Thanks.

[DrewAPicture]

Yeah, so @joyously's suggestion for using get_file_data() instead is a pretty elegant solution to the problem. I didn't prepare a patch but a code example should suffice.

Readme:

=== Test Plugin ===
Requires at least: 5.0
Tested up to: 5.1
Stable tag: 1.0.0
License: GPLv2
Requires WP: 5.0
Requires PHP: 7.3

Read with:

<?php
$headers = array(
    'requires_at_least' => 'Requires at least',
    'tested_up_to'      => 'Tested up to',
    'stable_tag'        => 'Stable tag',
    'license'           => 'License',
    'requires_wp'       => 'Requires WP',
    'requires_php'      => 'Requires PHP',
);

var_dump( get_file_data( __DIR__ . '/readme.txt', $headers ) );

Outputs:

Array
(
    [requires_at_least] => 5.0
    [tested_up_to] => 5.1
    [stable_tag] => 1.0.0
    [license] => GPLv2
    [requires_wp] => 5.0
    [requires_php] => 7.3
)

I think get_file_data() should be more than adequate. And bonus points for leveraging something already in core!

[afragen]

I’ll take a look but I remember something about that not quite working. I certainly could be wrong.

[afragen]

I admit to becoming confused in my exchange with @joyously as the initial point was using get_plugin_data on the plugin file.

I’ll see if I can make this work and refresh or comment either way.

Thanks @joyously @DrewAPicture

[afragen]

use get_file_data()

[afragen]

@joyously @DrewAPicture You guys were absolutely correct. Thanks for making me see the light.

Latest patch uses get_file_data(). Seems to work as expected.

[DrewAPicture]

Replying to afragen:

@joyously @DrewAPicture You guys were absolutely correct. Thanks for making me see the light.

Latest patch uses get_file_data(). Seems to work as expected.

All of the credit goes to @joyously, I just tested the theory :-)

[afragen]

Drew, that’s why I listed Joy first. 😉

[TimothyBlynJacobs]

Looking at .14

1. Patch applies cleanly.
2. The get_plugin_validation_data() header should be updated to reference get_file_data instead of the readme parser.
3. The $validation_headers is invalid. It has duplicated array keys. While less clean, the array keys should probably be separate for the tested|required and the tested|required up to header variants and then merged back together with whichever variant is better.
4. I think the null === $plugin_data check needs to either be relaxed or removed. I'd think the plugin headers should take priority and thus that code branch should always run, but not sure, either way the expected precedence should be documented. If just opting to relax the checks, it should also verify that the headers are not empty strings.
5. When calling get_plugin_data, can we disable $markup and $translate?
6. Document the return type. Should we return the tested up to values? It seems like they aren't used anywhere else in the patch and aren't provided by the plugin header.
7. Consider making the return type less ambiguous by returning requires_wp instead of just requires.
8. I'd update validate_plugin_requirements() to return true|WP_Error. The error would detail which requirements weren't met.
9. In concert with 7 update the error message in activate_plugin to be more specific. Depending on the implementation of validate_plugin_requirements it might make sense to just return that WP_Error object. Otherwise, if activate_plugin returns its own WP_Error, I think the error code could be more specific. Maybe unmet_requirements?

[afragen]

Updated from feedback

[afragen]

43992.15.diff​ based upon feedback below. Comments inline.

Replying to TimothyBlynJacobs:

Looking at .14

1. Patch applies cleanly.
2. The get_plugin_validation_data() header should be updated to reference get_file_data instead of the readme parser.

done

3. The $validation_headers is invalid. It has duplicated array keys. While less clean, the array keys should probably be separate for the tested|required and the tested|required up to header variants and then merged back together with whichever variant is better.

Shortened array to only 2 essential components.

4. I think the null === $plugin_data check needs to either be relaxed or removed. I'd think the plugin headers should take priority and thus that code branch should always run, but not sure, either way the expected precedence should be documented. If just opting to relax the checks, it should also verify that the headers are not empty strings.

We can discuss the precedence. As indicated in the comment blocks the precedence if for data in readme.txt with a fallback to new plugin headers.

5. When calling get_plugin_data, can we disable $markup and $translate?

done

6. Document the return type. Should we return the tested up to values? It seems like they aren't used anywhere else in the patch and aren't provided by the plugin header.

return type is documented as array, extraneous headers are no longer queried.

7. Consider making the return type less ambiguous by returning requires_wp instead of just requires.

$requires is parameter passed into the is_php_compatible() or is_wp_compatible() functions. Using a more generic parameter simply made the function and code more similar.

8. I'd update validate_plugin_requirements() to return true|WP_Error. The error would detail which requirements weren't met.

I think we'll have to agree to disagree. Returning a boolean for makes more sense to me as there is no error to be generated. If the validation fails, a WP_Error is generated.

9. In concert with 7 update the error message in activate_plugin to be more specific. Depending on the implementation of validate_plugin_requirements it might make sense to just return that WP_Error object. Otherwise, if activate_plugin returns its own WP_Error, I think the error code could be more specific. Maybe unmet_requirements?

I changed this to unmet_requirements

[afragen]

test for actionable data

[afragen]

Based upon a Twitter conversation, ​https://wordpress.slack.com/archives/C60K3MP2Q/p1552875771433600?thread_ts=1552875627.432000&cid=C60K3MP2Q

43992.16.diff​ tests for actionable data and replaces empty data in the readme.txt file with data from the plugin headers, if present.

This still give precedence to data in the readme.txt.

[afragen]

simpler logic

[TimothyBlynJacobs]

Updated patch looks good to me.

I'm not sure what the right solution is for the validate_plugin_requirements function. It seems like it'd be helpful for that to return which requirement failed. I agree WP_Error doesn't semantically make sense.

[desrosj]

This is testing pretty well for me. After some of the items below are solidified I can add some unit tests.

At first look, the thought that "we should pass a $context of plugin or plugin_validation to the get_file_data() call in get_plugin_validation_data()" came to mind. This would allow extra headers to be added and detected. But, if this was added, then a filter would need to be added to the end of get_plugin_validation(). Otherwise, detecting any additional headers would be useless. I can't think of a good use case right now, though, so let's leave it as is.

In the current state, any empty headers in the readme.txt will be ignored in favor of the plugin's headers, even if they are defined and empty. Just confirming that this is intended.

Can you confirm that I am understanding the precedence correctly:

• Non empty values defined in readme.txt.
• Plugin file header values when readme.txt does not exist.
• When readme.txt exists but has empty (or undefined) values for either header, the empty values will fall back to the plugin file headers.

I am also a little concerned with the function names is_wp_compatible() and is_php_compatible() being too generic. I did some brief searching around, and is_wp_compatible() seems to be pretty common among plugins. I am mostly seeing is_wp_compatible() as private or protected class methods, but I'd like a few more opinions on whether we should adjust the names a bit.

For the unmet_requirements error, maybe the approach could be:

• Add a new function that returns a boolean indicating a plugin can be activated.
• validate_plugin_requirements() returns an array indicating if PHP passed, WP passed, and if either fails, a string with the correct contextual error message.

Something like this could let us have different messages for each scenario. Examples: Plugin does not meet minimum WordPress requirements., Plugin does not meet minimum PHP requirements., Plugin does not meet minimum WordPress and PHP requirements..

Some other minor items:

• @uses ​should no longer be used within PHP docblocks. get_plugin_validation_data() description needs to be revised to be more clear about priority that data is used.
• Can the short description for get_plugin_validation_data() be more specific about what is being validated?

[afragen]

Replying to desrosj:

This is testing pretty well for me. After some of the items below are solidified I can add some unit tests.

At first look, the thought that "we should pass a $context of plugin or plugin_validation to the get_file_data() call in get_plugin_validation_data()" came to mind. This would allow extra headers to be added and detected. But, if this was added, then a filter would need to be added to the end of get_plugin_validation(). Otherwise, detecting any additional headers would be useless. I can't think of a good use case right now, though, so let's leave it as is.

I only added the headers we need to check. At some point we could add a filter to this array if it's necessary. But I also not sure of a use case.

In the current state, any empty headers in the readme.txt will be ignored in favor of the plugin's headers, even if they are defined and empty. Just confirming that this is intended.

Can you confirm that I am understanding the precedence correctly:

• Non empty values defined in readme.txt.
• Plugin file header values when readme.txt does not exist.
• When readme.txt exists but has empty (or undefined) values for either header, the empty values will fall back to the plugin file headers.

So both get_file_headers() and get_plugin_headers() will return an empty string if the header is missing or has no value.

Given that readme.txt files are required for the plugin directory any data existing here takes precedence. If a value exists in readme.txt it will be used. If a value doesn't exist in readme.txt then we look to the plugin headers to see if a value exists. If it does then we will use it. If no value exists in either place the return value would be an empty string.

Currently we are very permissive and an empty string automatically denotes a valid compatibility test. We can always change this in the future via the functions below.

I am also a little concerned with the function names is_wp_compatible() and is_php_compatible() being too generic. I did some brief searching around, and is_wp_compatible() seems to be pretty common among plugins. I am mostly seeing is_wp_compatible() as private or protected class methods, but I'd like a few more opinions on whether we should adjust the names a bit.

A search of ​WPDirectory for is_wp_compatible shows instances of the function name in only 20 different plugins and all seem to be namespaced.

A search of ​WPDirectory for is_php_compatible shows instances of the function name in only 17 different plugins and all seem to be namespaced.

For the unmet_requirements error, maybe the approach could be:

• Add a new function that returns a boolean indicating a plugin can be activated.
• validate_plugin_requirements() returns an array indicating if PHP passed, WP passed, and if either fails, a string with the correct contextual error message.

Something like this could let us have different messages for each scenario. Examples: Plugin does not meet minimum WordPress requirements., Plugin does not meet minimum PHP requirements., Plugin does not meet minimum WordPress and PHP requirements..

Let me look into this.

Some other minor items:

• @uses ​should no longer be used within PHP docblocks. get_plugin_validation_data() description needs to be revised to be more clear about priority that data is used.
• Can the short description for get_plugin_validation_data() be more specific about what is being validated?

OK

[afragen]

Update docblock

[afragen]

Replying to desrosj:

For the unmet_requirements error, maybe the approach could be:

• Add a new function that returns a boolean indicating a plugin can be activated.
• validate_plugin_requirements() returns an array indicating if PHP passed, WP passed, and if either fails, a string with the correct contextual error message.

Something like this could let us have different messages for each scenario. Examples: Plugin does not meet minimum WordPress requirements., Plugin does not meet minimum PHP requirements., Plugin does not meet minimum WordPress and PHP requirements..

If more granular error messaging is desired I think that's very achievable for 5.2.1. I agree that it's likely to require a function that returns the correct messaging.

[desrosj]

I think more granular error messaging in 5.2.1/5.3 can work.

For the functions, let's use wp_is_wp_compatible() and wp_is_php_compatible().

[afragen]

update function names to wp_is_wp_compatible() and wp_is_php_compatible()

[afragen]

add plugin name to data returned, prep for better messaging

[desrosj]

This is required prior to beta. Marking as high priority to receive a 2nd opinion.

[SergeyBiryukov]

• I'd like to see specific error messages for not meeting minimum WP version requirement vs. PHP version, rather than "WordPress and/or PHP". validate_plugin_requirements() could return WP_Error on error, I agree with @TimothyBlynJacobs in comment:33 here. That can probably be adjusted after beta though.
• Introducing wp_is_*_compatible() wrappers doesn't seem to bring a huge benefit at a glance, as there are only two or three of these checks in core. Maybe it will be easier to see once the other instances are updated to use the wrappers. I agree they should be prefixed with wp_, just in case.

Looks good otherwise.

[afragen]

Replying to SergeyBiryukov:

• I'd like to see specific error messages for not meeting minimum WP version requirement vs. PHP version, rather than "WordPress and/or PHP". validate_plugin_requirements() could return WP_Error on error, I agree with @TimothyBlynJacobs in comment:33 here. That can probably be adjusted after beta though.

It was my plan to fix this during beta. Do you want me to try and do it before commit?

• Introducing wp_is_*_compatible() wrappers doesn't seem to bring a huge benefit at a glance, as there are only two or three of these checks in core. Maybe it will be easier to see once the other instances are updated to use the wrappers. I agree they should be prefixed with wp_, just in case.

Search core for $compatible_wp and $compatible_php. It will also become most beneficial if/when the decision is made to require the Requires at least and Requires PHP headers in the readme. Then all plugin will be appropriately tested as opposed to allowing those without the headers to pass on through as valid.

Looks good otherwise.

Thanks.

[afragen]

granular messaging added

[afragen]

oops, missed @param in docblock

[afragen]

now actually include the last fix in the patch :facepalm:

[SergeyBiryukov]

For the first iteration, I'd like to simplify things a bit.

43992.23.diff​ reads the Requires at least and Requires PHP headers from both readme.txt and the main plugin file, with readme.txt taking precedence.

Per comment:23, this was done for plugins not hosted on w.org that may not have a valid readme.txt file.

However, on w.org these headers are specific to readme.txt, adding them to the plugin file does not have any effect.

Since we don't generally have any special treatment for plugins not hosted on w.org anywhere else in core, I wonder if by reading the headers from both places in this particular function (and advertising them as plugin headers vs. readme headers) we make things more complicated and confusing to plugin authors.

43992.24.diff​ is a simpler implementation that only reads the headers from readme.txt and makes validate_plugin_requirements() more consistent with validate_plugin(). For Beta 1, I think we could go with that, and then iterate further as needed.

[SergeyBiryukov]

In 44978:

Plugins: Block plugin activation if it requires a higher version of PHP or WordPress.

Introduce validate_plugin_requirements() for validating a plugin's WordPress and PHP version requirements.

Introduce wp_is_wp_compatible() and wp_is_php_compatible() for checking compatibility with the current WordPress or PHP version.

Props afragen, joyously, DrewAPicture, TimothyBlynJacobs, desrosj, flixos90, SergeyBiryukov.
See #43992.

[SergeyBiryukov]

In 44979:

PHPCS: Fix WPCS violations in [44978].

See #43992.

[SergeyBiryukov]

In 44981:

Plugins: Remove wp_is_wp_compatible() and wp_is_php_compatible() functions added in [44978] for now, to discuss use cases and better naming.

See #46599, #43992.

[JeffPaul]

Pinging @joshuawold for input/opinion as the Plugins component maintainer.

[schlessera]

Looks good and can be closed as far as the core-php is concerned.

[afragen]

See #46938

[afragen]

Replying to johnbillion:
Thanks, I didn’t seem to have the option to set to fixed. 😉

[SergeyBiryukov]

In 47573:

Themes: Block theme activation if it requires a higher version of PHP or WordPress.

Introduce validate_theme_requirements() for validating a theme's WordPress and PHP version requirements.

Follow-up to [44978] and [45546] for plugins.

Props afragen, audrasjb, SergeyBiryukov.
See #43992.

[SergeyBiryukov]

In 47574:

Plugins: Simplify the logic of validate_plugin_requirements(), update documentation.

This updates the function for consistency with validate_theme_requirements().

Follow-up to [44978], [45546], [47573].

Fixes #43992.

[SergeyBiryukov]

[47573] and [47574] were meant for #49334.