Skip to content

Update PHP compatibility sniff to PHP 7.4 #36

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

Closed
becw opened this issue Feb 18, 2022 · 6 comments · Fixed by #37
Closed

Update PHP compatibility sniff to PHP 7.4 #36

becw opened this issue Feb 18, 2022 · 6 comments · Fixed by #37

Comments

@becw
Copy link

becw commented Feb 18, 2022

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

<config name="testVersion" value="7.3-"/>

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
Copy link
Contributor

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
Copy link

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

@danepowell
Copy link
Collaborator

danepowell commented Mar 4, 2022

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
Copy link
Contributor

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
Copy link
Contributor

Thanks for fixing the failing tests, @danepowell! I've made the change in #37. Let me know if you think we should cut a release.

@pfrenssen
Copy link

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 squizlabs/PHP_CodeSniffer#2197

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

Successfully merging a pull request may close this issue.

5 participants