Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#46938 closed enhancement (fixed)

Add Requires WP and Requires PHP headers for plugin compatibility tests

Reported by: afragen's profile afragen Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: servehappy has-patch needs-testing commit
Focuses: Cc:

Description

Per #43992 comment:47 the second iteration of plugin activation checking should consider taking into account plugins that do not have valid readme.txt files but instead have added additional plugin headers, Requires WP and Requires PHP to the main plugin file.

Precedence is given to headers from the readme.txt file.

This is most common in premium plugins and plugins hosted elsewhere, like GitHub.

This is a likely enough scenario that we should test for it.

Attachments (2)

46938.diff (3.1 KB) - added by afragen 5 years ago.
test for new plugin compatibility headers
46938.2.diff (3.1 KB) - added by afragen 5 years ago.
give precedence to plugin headers

Download all attachments as: .zip

Change History (34)

@afragen
5 years ago

test for new plugin compatibility headers

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


5 years ago

#2 @tobifjellner
5 years ago

Also suitable for simple one-file plugins!

#3 @swissspidy
5 years ago

Not sure how good of an idea it is to add support for these to the plugin headers. It's a bit odd to be able to support these in two places.

#4 @afragen
5 years ago

@swissspidy it’s really a fallback if there aren’t settings in a readme.txt file.

Many plugins already declare these. https://wpdirectory.net/search/01D8HY8CV3HB0ZBZYGVZ2V6E8K

#5 @afragen
5 years ago

This might also be useful for mu-plugins. Haven’t tested yet.

#6 follow-up: @swissspidy
5 years ago

Many plugins already declare these

So they are technically doing_it_wrong at the moment, no?

This might also be useful for mu-plugins.

mu-plugins can't be activated or deactivated. They are always on. If someone adds an mu-plugin, they probably know what they're doing.

#7 in reply to: ↑ 6 @afragen
5 years ago

Replying to swissspidy:

Many plugins already declare these

So they are technically doing_it_wrong at the moment, no?

I’m not clear how adding a header comment is doing_it_wrong. I have personally used these headers in most of my plugins and check for them for precisely this purpose in GitHub Updater.

I think the decision is do we wish to create a method of testing plugins that are not in wp.org or do we not. I obviously have a preference.

Perhaps a few more core committers could give an up or down opinion.

This might also be useful for mu-plugins.

mu-plugins can't be activated or deactivated. They are always on. If someone adds an mu-plugin, they probably know what they're doing.

Absolutely correct.

Thanks @swissspidy for your insight. It is greatly appreciated.

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


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

#13 follow-up: @Ipstenu
5 years ago

How will this impact non-WordPress.org hosted plugins that don't include a readme.txt?

Are we now going to require that ALL plugins use our headers?

#14 follow-ups: @Otto42
5 years ago

Realistically, I'm surprised that we added parsing of the readme.txt for these in the first place. They should have been in the plugin's main PHP file all along.

Historically, the readme.txt for plugins has only been used by the WordPress.org directory. This provided a clear separation. WordPress reads the PHP file, the directory reads the readme.txt file for other things not in the PHP file's headers.

So, my 2 cents is to instead add reading these headers to the plugin's PHP file, and give precedence to that instead of the readme.txt file.

With a possible long term goal being to undo this mistake, and eliminate reliance on the readme.txt file from core itself. Readme.txt files are *not* required files for plugins, and never have been. We cannot rely on them being present.

#15 in reply to: ↑ 13 @afragen
5 years ago

Replying to Ipstenu:

How will this impact non-WordPress.org hosted plugins that don't include a readme.txt?

It will provide a mechanism for non-wp.org hosted plugins to be properly validated for plugin activation based upon the compatibility requirements listed in the plugin headers.

Are we now going to require that ALL plugins use our headers?

I think in time we may choose to require some definitive statement in the plugin regarding WP and PHP compatibility. Whether that occurs in the plugin headers and/or a readme.txt hasn’t been determined.

#16 in reply to: ↑ 14 @afragen
5 years ago

Replying to Otto42:

Realistically, I'm surprised that we added parsing of the readme.txt for these in the first place. They should have been in the plugin's main PHP file all along.

Historically, the readme.txt for plugins has only been used by the WordPress.org directory. This provided a clear separation. WordPress reads the PHP file, the directory reads the readme.txt file for other things not in the PHP file's headers.

So, my 2 cents is to instead add reading these headers to the plugin's PHP file, and give precedence to that instead of the readme.txt file.

I initially wrote the patch giving the precedence to the currently available information. It should be easy to switch.

With a possible long term goal being to undo this mistake, and eliminate reliance on the readme.txt file from core itself. Readme.txt files are *not* required files for plugins, and never have been. We cannot rely on them being present.

While readme.txt files aren’t required; neither are the plugin headers. My thought is that as long as the compatibility information exists in one of these places it should be utilized and in time required.

#17 follow-up: @Ipstenu
5 years ago

I think in time we may choose to require some definitive statement in the plugin regarding WP and PHP compatibility. Whether that occurs in the plugin headers and/or a readme.txt hasn’t been determined.

That isn't what I meant. Let me rephrase :)

Are we going to require all plugins, be they hosted on WordPress.org or not, to have PHP compat in headers in order to be activated?

The subject of the ticket is "Prevent activation of incompatible plugins without valid readme.txt" (bolding mine)

I took that to mean "If a plugin doesn't have valid headers, do not activate." And if that's the case, I am firmly against this call. It would be a terrible experience for users. Non-org plugins don't have readmes. Home grown plugins often don't. We would do more harm there.

I don't have a horse in the race for readme or php files, though based on what Otto said, PHP sounds smarter in the long run. Especially since you have to have the headers if you want your plugin to look nice in the plugins listing :)

#18 in reply to: ↑ 17 @afragen
5 years ago

Replying to Ipstenu:

I think in time we may choose to require some definitive statement in the plugin regarding WP and PHP compatibility. Whether that occurs in the plugin headers and/or a readme.txt hasn’t been determined.

That isn't what I meant. Let me rephrase :)

Are we going to require all plugins, be they hosted on WordPress.org or not, to have PHP compat in headers in order to be activated?

No, we aren’t ready to make this a mandatory requirement. At some future point I can see this requirement to aid in more completely preventing plugin PHP fatals for PHP incompatibility.

The subject of the ticket is "Prevent activation of incompatible plugins without valid readme.txt" (bolding mine)

I took that to mean "If a plugin doesn't have valid headers, do not activate." And if that's the case, I am firmly against this call. It would be a terrible experience for users. Non-org plugins don't have readmes. Home grown plugins often don't. We would do more harm there.

I see this as an either/or choice. The PHP compatibility headers would be either in a readme.txt or the plugin headers. At this time there is no requirement for compatibility headers anywhere. As such if none exist a plugin is allowed to install, update, and activate.

The current is_php_version_compatible() and is_wp_version_compatible() are true if those values evaluate to true via the version_compare or if they don’t exist or are empty. This title was a bit of future guessing that someday a WP and PHP requirement might actually be required and the best (IMO) method of allowing this for the most users is the ability to have this information in the plugin headers.

I don't have a horse in the race for readme or php files, though based on what Otto said, PHP sounds smarter in the long run. Especially since you have to have the headers if you want your plugin to look nice in the plugins listing :)

The patch only seeks to allow for the Requires WP and Requires PHP headers to be utilized by the compatibility tests. Nothing more.

I do agree that a larger discussion about the requirement of such headers for plugin activation, etc. should be had at some point in the future. Currently I think we shouldn’t penalize anyone for not having the compatibility requirements noted. It is my opinion that both methods should continue to be acceptable now, and in the future, as data in the readme.txt and in plugin headers is a well established practice for WordPress plugins.

@Ipstenu @Otto42 I really appreciate your opinions and expertise in this area. Thank you for all you do for us in the Plugin Directory.

#19 @Ipstenu
5 years ago

I see this as an either/or choice. The PHP compatibility headers would be either in a readme.txt --

I'm not asking about the PHP compat headers at all at this moment. I'm asking for clarification about the intent of this ticket, becuase based on the name, I think you've crossed your streams. :)

I think you want to rename this trac ticket, because I get the impression you're meaning "Check PHP Compat before activation" and instead you said "Don't let plugins with bad readme headers activate." Those aren't at all the same thing.

To be clear:

  • Should we check PHP compatibility before activation? Yes
  • Should we prevent activation if compat is invalid (i.e. you have PHP 5.6 and the plugin says 7.1)? Yes
  • Should we prevent activation is the PHP compat header is missing? NO
  • Should we prevent activation if the readme.txt has mucky headers or is missing? NO

The patch only seeks to allow for the Requires WP and Requires PHP headers to be utilized by the compatibility tests. Nothing more.

Then yeah you absolutely need to rename this ticket, because it's very misleading. I recommend "Add Requires WP and Requires PHP headers to plugin compatibility tests"

And remember that lack of those headers, regardless of where they are (readme, php, a json file, don't care), means we warn but still permit activation.

#20 @afragen
5 years ago

  • Summary changed from Prevent activation of incompatible plugins without valid readme.txt to Add Requires WP and Requires PHP headers for plugin compatibility tests

Thanks @Ipstenu

And remember that lack of those headers, regardless of where they are (readme, php, a json file, don't care), means we warn but still permit activation.

We actually don’t warn we simply permit activation, but yes.

Last edited 5 years ago by afragen (previous) (diff)

#21 in reply to: ↑ 14 ; follow-up: @SergeyBiryukov
5 years ago

Replying to Otto42:

Realistically, I'm surprised that we added parsing of the readme.txt for these in the first place. They should have been in the plugin's main PHP file all along.

Historically, the readme.txt for plugins has only been used by the WordPress.org directory. This provided a clear separation. WordPress reads the PHP file, the directory reads the readme.txt file for other things not in the PHP file's headers.

Thanks for the context. Just to clarify, the Requires PHP header was introduced in #meta2952 for the directory first, so it made more sense to have it defined in the same place as Requires at least, which only exists in readme.txt. Otherwise it would be confusing and inconsistent to define requirement headers in two different places, one in readme.txt and one in the plugin's main PHP file, especially if each only works in that specific place.

So, my 2 cents is to instead add reading these headers to the plugin's PHP file, and give precedence to that instead of the readme.txt file.

If that's the preferred way forward, sounds good. Plugin Directory would need the same change as well for consistency.

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

@afragen
5 years ago

give precedence to plugin headers

#22 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.3

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


5 years ago

#24 @SergeyBiryukov
5 years ago

  • Keywords commit added; dev-feedback removed

For consistency with the readme.txt headers, I think Requires WP should be Requires at least, so that both headers (Requires at least and Requires PHP) could be supported in both places (readme.txt and plugin file). Looks good otherwise.

#25 @SergeyBiryukov
5 years ago

#28574 was marked as a duplicate.

#26 in reply to: ↑ 21 @SergeyBiryukov
5 years ago

Replying to SergeyBiryukov:

So, my 2 cents is to instead add reading these headers to the plugin's PHP file, and give precedence to that instead of the readme.txt file.

If that's the preferred way forward, sounds good. Plugin Directory would need the same change as well for consistency.

Created #meta4514 for the Plugin Directory.

#27 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 45546:

Plugins: When validating plugin's WordPress and PHP requirements, check for Requires at least and Requires PHP headers in the plugin's main PHP file.

This allows for blocking plugin activation if it requires a higher version of PHP or WordPress, and does not have a readme.txt file.

If the headers are defined in both readme.txt and the main plugin file, precedence is given to the plugin file.

Props afragen, Otto42, Ipstenu.
Fixes #46938.

#28 @spacedmonkey
5 years ago

  • Component changed from Plugins to Site Health

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


5 years ago

This ticket was mentioned in Slack in #meta by casiepa. View the logs.


5 years ago

#31 @dd32
5 years ago

#42980 was marked as a duplicate.

This ticket was mentioned in Slack in #hosting-community by mike. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.