acquia / coding-standards-php

PHP_CodeSniffer rules (sniffs) for Acquia coding standards
GNU General Public License v2.0
19 stars 13 forks source link

Update PHP compatibility sniff to PHP 7.4 #36

Closed becw closed 2 years ago

becw commented 2 years ago

Now that PHP 7.3 is no longer supported, is it time to update the PHP compatibility sniff?

https://github.com/acquia/coding-standards-php/blob/0b46d59ac8dc191cf85622eb4fc01267f641df53/src/Standards/AcquiaPHP/ruleset.xml#L89

Also -- I found that having the testVersion set here meant that I couldn't override it in my project's phpcs.xml -- I can only override it by specifying it on the command line (e.g. phpcs --runtime-set testVersion 7.4).

TravisCarden commented 2 years ago

That's a good thought, @becw. Unfortunately, Drupal 9 still officially support PHP 7.3, so module developers still need to as well, and for their sake we can't drop support for it, either. However, individual sites only need to support the version on their own servers, so it would be desirable for them to be able to override the default. @danepowell, should we move the config from src/Standards/AcquiaPHP/ruleset.xml to example/phpcs.xml.dist if we can verify that it's not currently overridable?

grasmash commented 2 years ago

I'd also like to override this for acquia/cli.

danepowell commented 2 years ago

This can be overridden in consuming projects, you just need to exclude the PHPCompatibility rule when including Acquia's own standards:

 <ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
          xsi:noNamespaceSchemaLocation="vendor/squizlabs/php_codesniffer/phpcs.xsd"
 >
-  <rule ref="AcquiaPHP"/>
+  <rule ref="AcquiaPHP">
+    <exclude name="PHPCompatibility"/>
+  </rule>
+  <config name="testVersion" value="7.4-"/>
+  <rule ref="PHPCompatibility">
+    <exclude name="PHPCompatibility.Extensions.RemovedExtensions.famRemoved"/>
+  </rule>
 </ruleset>

Having said that, I think we should update the rule in ORCA to 7.4. I understand that Drupal 9 still officially supports PHP 7.3, but Acquia does not. If we explicitly do not support PHP 7.3, that should take precedence over any other library's support policy (including Drupal's).

TravisCarden commented 2 years ago

Oh, that's a good point, @danepowell. I agree. I'll update this with the above pull request as soon as I can figure out why its tests are failing.

TravisCarden commented 2 years ago

Thanks for fixing the failing tests, @danepowell! I've made the change in https://github.com/acquia/coding-standards-php/pull/37. Let me know if you think we should cut a release.

pfrenssen commented 2 years ago

Also -- I found that having the testVersion set here meant that I couldn't override it in my project's phpcs.xml -- I can only override it by specifying it on the command line (e.g. phpcs --runtime-set testVersion 7.4).

I also noticed this. The config value can at the moment not be overridden except on the CLI. This behavior will be fixed in PHP_Codesniffer 4.x, ref https://github.com/squizlabs/PHP_CodeSniffer/issues/2197