Switch Acquia PHP to PSR12 #68
Conversation
| <!-- Superglobals are superbad. See linked issue for discussion. | ||
| @see https://github.com/acquia/coding-standards-php/issues/49 --> | ||
| <rule ref="SlevomatCodingStandard.Variables.DisallowSuperGlobalVariable" /> | ||
| <rule ref="SlevomatCodingStandard.Variables.DisallowSuperGlobalVariable"/> |
There was a problem hiding this comment.
These slevomat standards are some of my faves. If they're too opinionated or just not your jam, let me know.
There was a problem hiding this comment.
The relationship of these changes to Drupal coding standards is complex. We'd better test thoroughly. We may want to add a representative Drupal module to our tests. I also suggest working with @secretsayan to look into how our existing modules will be affected and make sure ORCA doesn't accidentally roll out the changes before we warn our users and providing an upgrade path, as it were.
| * [AcquiaPHP](src/Standards/AcquiaPHP/ruleset.xml) contains sniffs applicable to all PHP projects. | ||
| * [AcquiaDrupalStrict](src/Standards/AcquiaDrupalStrict/ruleset.xml) incorporates AcquiaPHP and adds all Drupal coding standards and best practices sniffs. Recommended for new Drupal projects and teams familiar with Drupal coding standards. | ||
| * [AcquiaDrupalTransitional](src/Standards/AcquiaDrupalTransitional/ruleset.xml) incorporates AcquiaPHP and adds Drupal core's own phpcs configuration, which is less strict than the official standards. Recommended for legacy Drupal codebases or teams new to Drupal coding standards. | ||
| * [AcquiaPHP](src/Standards/AcquiaPHP/ruleset.xml) is the preferred standard for all Acquia projects, including Drupal projects. It is based on PSR-12 and compatible with Drupal standards. In other words, any code meeting the AcquiaPHP standard will also meet Drupal standards, but will take advantage of new language features offered by PSR-12 and (soon) PER-2. |
There was a problem hiding this comment.
the preferred standard for all Acquia projects, including Drupal projects
Did I misunderstand your proposal in Slack? This sounds like AcquiaPHP is, in fact, a superset of AcquiaDrupalStrict, i.e., based on and superseding it. In other words, we just flipped the direction of dependence so that AcquiaPHP is on the top now instead of the bottom; we didn't actually decouple them. I think that if I were a new user I would intuit the opposite based solely on their names--especially since that's what it meant before. (And, you know, only two hard problems. 🙂)
Maybe we could play around with some form of AcquiaPreferred or something, which I think implies more of our intent and doesn't carry the historical baggage? Or maybe we explore (what I thought was your original proposal) making AcquiaPHP compatible with AcquiaDrupal* but distinct--such that anyone could choose just one or the other if they wanted--and then create a new AcquiaPreferred that just references both AcquiaDrupalStrict and AcquiaPHP.
| * [AcquiaPHP](src/Standards/AcquiaPHP/ruleset.xml) contains sniffs applicable to all PHP projects. | ||
| * [AcquiaDrupalStrict](src/Standards/AcquiaDrupalStrict/ruleset.xml) incorporates AcquiaPHP and adds all Drupal coding standards and best practices sniffs. Recommended for new Drupal projects and teams familiar with Drupal coding standards. | ||
| * [AcquiaDrupalTransitional](src/Standards/AcquiaDrupalTransitional/ruleset.xml) incorporates AcquiaPHP and adds Drupal core's own phpcs configuration, which is less strict than the official standards. Recommended for legacy Drupal codebases or teams new to Drupal coding standards. | ||
| * [AcquiaPHP](src/Standards/AcquiaPHP/ruleset.xml) is the preferred standard for all Acquia projects, including Drupal projects. It is based on PSR-12 and compatible with Drupal standards. In other words, any code meeting the AcquiaPHP standard will also meet Drupal standards, but will take advantage of new language features offered by PSR-12 and (soon) PER-2. |
There was a problem hiding this comment.
It is based on PSR-12 and compatible with Drupal standards.
Since there is actually some incompatibility between these two, this needs to be a "but" proposition. However we word it exactly, it needs to communicate that it's based on PSR-12* but is compatible with AcquiaDrupalStrict and where they conflict, AcquiaDrupalStrict wins.
*And does "based on" mean more like "inspired by" or "we included all of it except in cases of conflicts with Drupal standards"?
| <rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint" /> | ||
| <rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint" /> | ||
| <rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint" /> | ||
| <rule ref="SlevomatCodingStandard.Arrays.AlphabeticallySortedByKeys"/> |
There was a problem hiding this comment.
I like this one in theory, but in practice, it may be infeasible--I believe it would fail legitimate Drupal Form API and render arrays, for example.
| <rule ref="SlevomatCodingStandard.Arrays.MultiLineArrayEndBracketPlacement"/> | ||
| <rule ref="SlevomatCodingStandard.Arrays.SingleLineArrayWhitespace"/> | ||
| <rule ref="SlevomatCodingStandard.Arrays.TrailingArrayComma"/> | ||
| <rule ref="SlevomatCodingStandard.Classes.RequireConstructorPropertyPromotion"/> |
There was a problem hiding this comment.
Is this sniff smart enough to ignore PHP 7? Because constructor property promotion was added in PHP 8.0, but we don't require PHP 8, and believe it or not, neither do any of our dependencies*! Based on the documentation, it looks like it's smart enough, but we should double check if we haven't. If it's not smart enough, we need to declare a requirement on php:^8.0 in our composer.json; because as it stands, we only (transitively) require >=7.2.5.
*
$ composer why php
dealerdirect/phpcodesniffer-composer-installer v0.7.2 requires php (>=5.3)
drupal/coder 8.3.16 requires php (>=7.1)
phpcompatibility/php-compatibility 9.3.5 requires php (>=5.3)
phpstan/phpdoc-parser 1.7.0 requires php (^7.2 || ^8.0)
sirbrillig/phpcs-variable-analysis v2.11.7 requires php (>=5.4.0)
slevomat/coding-standard 8.4.0 requires php (^7.2 || ^8.0)
squizlabs/php_codesniffer 3.7.1 requires php (>=5.4.0)
symfony/deprecation-contracts v2.5.2 requires php (>=7.1)
symfony/polyfill-ctype v1.26.0 requires php (>=7.1)
symfony/yaml v5.4.11 requires php (>=7.2.5)
| <exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/> | ||
| <rule ref="PSR2.ControlStructures.SwitchDeclaration"> | ||
| <properties> | ||
| <property name="indent" value="2" /> |
There was a problem hiding this comment.
Should we keep all of our overrides for Drupal compatibility in one place? Or at least put an explanatory comment with them? Just a thought.
| <rule ref="SlevomatCodingStandard.Arrays.TrailingArrayComma"/> | ||
| <rule ref="SlevomatCodingStandard.Classes.RequireConstructorPropertyPromotion"/> | ||
| <rule ref="SlevomatCodingStandard.Commenting.DeprecatedAnnotationDeclaration"/> | ||
| <rule ref="SlevomatCodingStandard.Commenting.DisallowCommentAfterCode"/> |
There was a problem hiding this comment.
Aren't some of these already a part of the Drupal standard? If so, maybe we should put them together with the Drupal sniffs, even though they're supplied by other standards. I feel like that's more significant. I suppose, by that logic, we should put everything that's not from PSR-12 or Drupal into an "Acquia-specific" section. (But I'm open to being persuaded otherwise.)
| <rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint"> | ||
| <exclude name="SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingTraversableTypeHintSpecification"/> | ||
| </rule> | ||
| <rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint"/> | ||
| <rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint"/> |
There was a problem hiding this comment.
Am I wrong in thinking that if a user had custom code extending a Drupal core class that violated this rule the custom code would have to violate it, too, as a matter of inheritance? If so, there are probably a lot of cases like this.
| <!-- Disable some error messages that we do not want. --> | ||
| <rule ref="PEAR.Files.IncludingFile.UseIncludeOnce"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Files.IncludingFile.UseInclude"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Files.IncludingFile.UseRequireOnce"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Files.IncludingFile.UseRequire"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature.OpeningIndent"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Functions.ValidDefaultValue"/> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature"/> | ||
| <!-- The sniffs inside PEAR.Functions.FunctionCallSignature silenced below are | ||
| also silenced in Drupal CS' ruleset.xml. The code below is a 1-to-1 copy | ||
| from that file. --> | ||
| <!-- Disable some error messages that we already cover. --> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <!-- Disable some error messages that we do not want. --> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature.Indent"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature.CloseBracketLine"> | ||
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="PEAR.Functions.FunctionCallSignature.EmptyLine"> | ||
| <severity>0</severity> | ||
| </rule> |
There was a problem hiding this comment.
Just for my understanding, what's the reasoning behind this change? To be honest, I'm not certain why they were there in the first place other than that we copied them from Drupal core.
| <rule ref="SlevomatCodingStandard.Commenting.UselessFunctionDocComment"/> | ||
| <rule ref="SlevomatCodingStandard.Commenting.UselessInheritDocComment"/> |
There was a problem hiding this comment.
I think these conflict with Drupal's standards.
There was a problem hiding this comment.
If we're going to allow my personal opinions to dictate the Acquia PHP standard (and honestly, why shouldn't we? 😁 ), then I see no choice but to maintain separate standards for Acquia PHP and Drupal. We can try to keep them somewhat interchangeable, but Drupal standards dictate a few especially silly things (this being one example) that I just can't abide.
Are you onboard with Acquia PHP being based on PSR-12 and Acquia Drupal Transitional / Strict being based on Drupal / Drupal Practice? The question is how much to make them look like each other. To the greatest extent possible, I'd say Acquia PHP should be pure PSR-12 with only minor additions, and Acquia Drupal should be pure Drupal.
There was a problem hiding this comment.
Are you onboard with Acquia PHP being based on PSR-12 and Acquia Drupal Transitional / Strict being based on Drupal / Drupal Practice?
Yes, I think so. It's unfortunate that Drupal's coding standards are so different from the rest of the PHP world, and it would be less than ideal for Acquia employees who work on both Drupal contrib and non-Drupal projects to switch back and forth. But coding standards aren't just for decoration--they train on best practices, too. And bad ones can prevent growth in that area and rob developers of transferrable skills. We should help Drupalers be good Drupalers and PHP generalists be good generalists.
We need to discuss the implications and the plan. It could be disruptive depending on how we rolled it out. But if we give people the option to choose which one they want to use for non-Drupal projects, that could mitigate most of the risk. We should probably bring it up to a few stakeholders at various levels to see what they think.
|
See #70 |
This is a ground-up rewrite of the Acquia PHP standard to be based on PSR-12 and compatible with Drupal standards. In other words, any code adhering to Acquia PHP will also adhere to Drupal standards, but not necessarily vice-versa.
This means that Acquia PHP is a stricter subset of PSR-12 and the Drupal standards, which is a paradigm shift from its original implementation as a looser standard (and hence, base of) the Drupal standards. This warrants a new major release.
PER isn't available in PHPCS yet, but the good news is it's mostly backwards-compatible (i.e., only contains additions for new PHP features) and can we can basically swap it in for PSR-12 when it lands:
PHPCSStandards/PHP_CodeSniffer#29
The best way to test this is against projects already on the latest version of the AcquiaPHP or AcquiaDrupal* standards.
Dependency changes