Skip to content
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

Minimal PHP version #228

Open
zimnyd opened this issue Jul 18, 2019 · 11 comments
Open

Minimal PHP version #228

zimnyd opened this issue Jul 18, 2019 · 11 comments
Labels

Comments

@zimnyd
Copy link

zimnyd commented Jul 18, 2019

https://github.com/WPTRT/WPThemeReview/blob/5a223995395a41c05c9e35cb3651f6d9e60ff786/WPThemeReview/ruleset.xml#L8

Please change minimal PHP higher than 5.2, because even minimal php for The WPThemeReview Standard is 5.4 and now for Wordpress the minimal PHP is 5.6 See:
https://wordpress.org/news/2019/04/minimum-php-version-update/

@joyously
Copy link

The minimal version should reflect what the code needs, not what other code needs.
Or is this config for the version to be tested?

It doesn't make sense to bump it up if it's not needed, because those people trying to move to more recent versions may need to run the tool in order to get there.

@jrfnl
Copy link
Contributor

jrfnl commented Jul 18, 2019

Themes normally support multiple versions of WordPress, so while the minimum PHP requirement for WP 5.2 is now PHP 5.6.20, for WP < 5.2, it still is PHP 5.2.6.

The default presumption made by WPThemeReview is that a theme will support up to three WP versions before the current WP version.

So, no, at this moment it is too early to change the default testVersion configuration. Once WP 5.6 comes out, we can revisit this.

For your own projects, if you so choose, you can overrule the setting by using --runtime-set testVersion 5.6- when running the sniffs from the command-line.

As for that the minimum PHP version for running WPThemeReview is PHP 5.4: that's neither here nor there. While the sniffs are designed to check code in WP Themes, for running the sniffs, there is no dependency on WordPress, only on PHP_CodeSniffer and the current version of PHP_CodeSniffer has a minimum requirement of PHP 5.4.

@jrfnl jrfnl added the wontfix label Jul 18, 2019
@szepeviktor
Copy link

szepeviktor commented Aug 20, 2019

@jrfnl Is there a - dirty - workaround to support only PHP 5.3?
Thanks.

A better one than sed -e '/name="testVersion"/d' -i vendor/wptrt/wpthemereview/WPThemeReview/ruleset.xml

@jrfnl
Copy link
Contributor

jrfnl commented Aug 20, 2019

Is there a - dirty - workaround to support only PHP 5.3?

@szepeviktor You mean for the sniffs to run on PHP 5.3 or for your code to be tested for compatibility with PHP 5.3 ?

If the former, no, PHP 5.4 is the minimum PHP version needed to run the sniffs.

If the latter, you can overrule the testVersion from the command-line as stated above:

# Only check for compatibility with PHP 5.3
phpcs . --standard=WPThemeReview --runtime-set testVersion 5.3

# Check for compatibility with PHP 5.3 and higher
phpcs . --standard=WPThemeReview --runtime-set testVersion 5.3-

@szepeviktor
Copy link

Thank you Juliette.

@WraithKenny
Copy link

Is there a way to override this in your own theme's phpcs.xml file?

I've tried to include the config value in my ruleset, but it seems to be ignored. I'd love to be able to use this, but I use the file based config, not the command line.

@WraithKenny
Copy link

@WraithKenny
Copy link

Seems that, currently, it's not possible to change the minimum php value from a ruleset that includes the theme review ruleset.

That's super disappointing.

@WraithKenny
Copy link

To anyone who finds these comments here, the solution is to create an extra xml file (test-version-override.xml), with the contents

<?xml version="1.0"?>
<ruleset>
	<config name="testVersion" value="5.6-"/>
</ruleset>

and include it after the WPThemeReview rule:

	...
	<rule ref="WPThemeReview"/>
	<rule ref="./test-version-override.xml"/>

@dingo-d
Copy link
Member

dingo-d commented Oct 24, 2019

This is a known bug and Juliette mentioned how to override it in her comment above.

Another thing you can do is what we've done in Twenty Twenty theme:

https://github.com/WordPress/twentytwenty/blob/f257dfff755c415dadc56a2c70a412421f39aad7/composer.json#L25-L28:

"scripts": {
  "checkcs": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs --runtime-set testVersion 5.6-",
  "fixcs": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcbf --runtime-set testVersion 5.6-",
  "lint": "@php ./vendor/bin/parallel-lint --exclude .git --exclude vendor ."
},

Then run your sniffs using composer checkcs 🙂

@WraithKenny
Copy link

I use VSCode + ikappas.phpcs extension, which is set up to look for a phpcs.xml in the project root, and checks/highlights errors and warnings while typing, rather than having to running the command occasionally.

Your solution is good for other workflows/environments, and especially Theme Reviewing, definitely! But if anyone needs my workaround to get their own setup working, I hope it's useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants