WPTT / theme-sniffer

Theme Sniffer plugin using sniffs.
MIT License
269 stars 3 forks source link

closes #146, fixes #155 #157

Closed timelsass closed 5 years ago

timelsass commented 5 years ago

This PR adds support for minimum PHP versions and preselecting the one based on the theme's specifications, or WP Core version they are using.

It does the following:

  1. Preselect the minimum PHP Version if it's provided by a theme in the theme's readme.txt using the format supported for plugins' readme.txt. ex: Requires PHP: 5.4
  2. This will check for the transient data stored by wp_version_check() in php_check_{$key}. This function was a more recent addition to core (5.1), so the code was included for the actual API call if the function doesn't exist. This will allow us to still get an appropriate required PHP version for the core WP version the user is on as a response without relying on that WP version+ until enough time has passed.
  3. If unable to communicate with wp.org API then it will use 5.2 as the minimum. I think WP 5.5 would be the point in which we could fully get rid of 5.2 from Theme Sniffer since themes can support 3 major WordPress versions for backwards compat, and we should continue to provide sniffs for that code at least until that point.

I believe this solution will start to get authors to declare their minimum required PHP version via their readme.txt, so this is something that I think is a good thing. This should also help signal to reviewers what to look for. Currently with this PR using WP 5.1.1, if the theme doesn't supply a required PHP version, the preselected option will be 5.2 based on the WP.org api call, which returns 5.2.4. If you see a different version, then that means the author has declared a higher PHP version in their readme.txt, so you should check the entry points for PHP compatibility up until they perform a version check.

Once WP 5.2 rolls out, then the default selected option from the API call to core will preselect the option for 5.6 if the user updates their WP version, and hasn't specified in their readme.txt that they are supporting 5.2.

As a question though @carolinan and @dingo-d do you think that this should rely on the API call alone instead of the transient cached via php_check for the particular install, or call first to get latest WP version, and use that? I didn't think about that until now, but it may be a worthwhile addition to prevent any confusion if people are switching between various WP versions. This would force the minimum required version (if not overridden by theme readme.txt) to always preselect based on the latest core release. The advantage the current way has is that it's based on the WP version the user is currently running, which seems more useful from the aspect of this running as a plugin for authors who might be using it for things other than wp.org repo submissions, and enforcing coding standards on custom themes for sites that they can't update WP core versions on for whatever reason. Not really sure of all the scenarios that exist, or which method is better, so if you guys have preference let me know and I can make some changes.

dingo-d commented 5 years ago

One thing I'm wondering is: wasn't the PHP 5.2-5.5 support dumped earlier this March?

Also, you mentioned that the check is based on a WP core version. Shouldn't it be based on the server that is running the WP? Or maybe I misunderstood something.

timelsass commented 5 years ago

One thing I'm wondering is: wasn't the PHP 5.2-5.5 support dumped earlier this March?

No :smiley: WP5.2 is the targeted WP Version for this

Also, you mentioned that the check is based on a WP core version. Shouldn't it be based on the server that is running the WP? Or maybe I misunderstood something.

Yes that's pretty much what my question is about - the PR is based on the user's WP Core Version that they are running. The API calls to .org are just used to get the "official" php version constraints for a particular WP version. This will prevent us from having to manually update the PHP version in the future. If you think it should be based on the server that is running the WP, then the PR should be good to go. My question was more or less should it rely on the user's installed WP version, or should we make another call to get the latest WP version that's available and base off of that (regardless if the user has not updated their install).

dingo-d commented 5 years ago

No 😃 WP5.2 is the targeted WP Version for this

Oh 🙈

My question was more or less should it rely on the user's installed WP version, or should we make another call to get the latest WP version that's available and base off of that (regardless if the user has not updated their install).

Hmmm this is a good question. On one hand if the user is on a certain WP version it's good for him to test against that, on the other hand we should try to 'force' people to use the latest versions, so it might be a good idea to check the latest WP version and base it off of that 🤷‍♂️

joyously commented 5 years ago

You're only talking about the default value, aren't you? The user can still change it. Just use the current site's WP for PHP min, as default when the readme doesn't have a value.

timelsass commented 5 years ago

You're only talking about the default value, aren't you? The user can still change it. Just use the current site's WP for PHP min, as default when the readme doesn't have a value.

@dingo-d makes a good point about encourage users to use the latest, but I think I am am leaning towards this direction too if everyone else agrees with it.

Also as @joyously states, really it's just for convenience of the default selection - a reviewer would (or probably should) be doing a fresh install that would have the latest WP version , so I think it makes sense to leave it how it is. As an author, I do test on older WP versions for compatibility, and I think it does make sense for the plugin to respect those version defaults after testing and using the feature a bit more today.