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.

Sign up for GitHub

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

Switch Acquia PHP to PSR12 #68

Closed
wants to merge 14 commits into from
Closed

Switch Acquia PHP to PSR12 #68

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
430d8c2
Switch Acquia PHP to PSR12
danepowell Mar 28, 2024
e66e673
Add edge standards
danepowell Mar 28, 2024
49a1d94
typo
danepowell Mar 28, 2024
92d5cab
Add slevomat standards
danepowell Mar 28, 2024
d484975
fix test
danepowell Mar 28, 2024
94d545e
fix tests
danepowell Mar 28, 2024
7d9894a
fix tests
danepowell Mar 28, 2024
a78ddaa
fix tsets
danepowell Mar 28, 2024
51835ae
Fix class declaration
danepowell Mar 29, 2024
3dad619
fix function declarations
danepowell Mar 30, 2024
b2d409a
functioncallsignature
danepowell Apr 1, 2024
9105771
control structure spacing
danepowell Apr 1, 2024
4474fe7
controlsignature
danepowell Apr 1, 2024
3b802da
cleanup
danepowell Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/Standards/AcquiaEdge/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,4 @@
<!-- Acquia PHP sniffs -->
<rule ref="AcquiaPHP"/>

<!-- SlevomatCodingStandard sniffs -->
<rule ref="SlevomatCodingStandard.Commenting.DocCommentSpacing" />
<rule ref="SlevomatCodingStandard.Commenting.EmptyComment" />

</ruleset>
144 changes: 37 additions & 107 deletions src/Standards/AcquiaPHP/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,19 @@

<description>Acquia's PHP coding standards.</description>

<rule ref="PSR12">
<exclude name="Generic.WhiteSpace.ScopeIndent"/>
<exclude name="Generic.PHP.LowerCaseConstant"/>
</rule>

<!-- Drupal sniffs -->
<!-- These provide PHPCBF integration, unlike their Squiz equivalents -->
TravisCarden marked this conversation as resolved.
Show resolved Hide resolved
<rule ref="Drupal.Classes.ClassDeclaration" />
<rule ref="Drupal.Classes.UnusedUseStatement"/>
<!-- https://www.php-fig.org/psr/psr-12/#5-control-structures -->
<rule ref="Drupal.ControlStructures.ControlSignature" />
<rule ref="Drupal.Functions.FunctionDeclaration" />
<rule ref="Drupal.Scope.MethodScope"/>
<rule ref="Drupal.Commenting.InlineComment"/>
<rule ref="Drupal.WhiteSpace.ScopeIndent"/>

<!-- Generic sniffs -->
<rule ref="Generic.Arrays.DisallowLongArraySyntax"/>
<rule ref="Generic.Files.ByteOrderMark"/>
<rule ref="Generic.Files.EndFileNewline" />
<rule ref="Generic.Files.LineEndings"/>
<rule ref="Generic.Formatting.SpaceAfterCast"/>
<rule ref="Generic.Functions.FunctionCallArgumentSpacing"/>
<rule ref="Generic.Functions.OpeningFunctionBraceKernighanRitchie">
<properties>
<property name="checkClosures" value="true"/>
Expand All @@ -33,10 +29,7 @@
<rule ref="Generic.NamingConventions.ConstructorName"/>
<rule ref="Generic.NamingConventions.UpperCaseConstantName"/>
<rule ref="Generic.PHP.DeprecatedFunctions"/>
<rule ref="Generic.PHP.DisallowShortOpenTag"/>
<rule ref="Generic.PHP.LowerCaseKeyword"/>
<rule ref="Generic.PHP.UpperCaseConstant"/>
<rule ref="Generic.WhiteSpace.DisallowTabIndent"/>

<!-- Internal sniffs -->
<rule ref="Internal.NoCodeFound">
Expand All @@ -62,34 +55,6 @@
<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>

<!-- PHP Compatibility sniffs -->
<!-- The lowest version of PHP supported by both Drupal and Acquia Cloud.
Expand All @@ -102,26 +67,40 @@
<exclude name="PHPCompatibility.Extensions.RemovedExtensions.famRemoved"/>
</rule>

<!-- PSR-2 sniffs -->
<rule ref="PSR2.Classes.PropertyDeclaration">
<exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/>
<!-- SlevomatCodingStandard sniffs -->
<rule ref="SlevomatCodingStandard.Arrays.AlphabeticallySortedByKeys"/>
Copy link
Contributor

@TravisCarden TravisCarden Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.DisallowImplicitArrayCreation"/>
<rule ref="SlevomatCodingStandard.Arrays.MultiLineArrayEndBracketPlacement"/>
<rule ref="SlevomatCodingStandard.Arrays.SingleLineArrayWhitespace"/>
<rule ref="SlevomatCodingStandard.Arrays.TrailingArrayComma"/>
<rule ref="SlevomatCodingStandard.Classes.RequireConstructorPropertyPromotion"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

<rule ref="SlevomatCodingStandard.Commenting.DeprecatedAnnotationDeclaration"/>
<rule ref="SlevomatCodingStandard.Commenting.DisallowCommentAfterCode"/>
Copy link
Contributor

@TravisCarden TravisCarden Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Commenting.DocCommentSpacing"/>
<rule ref="SlevomatCodingStandard.Commenting.EmptyComment"/>
<rule ref="SlevomatCodingStandard.Commenting.ForbiddenAnnotations">
<properties>
<property name="forbiddenAnnotations" type="array" value="@author,@created,@version,@package,@copyright,@license,@throws"/>
</properties>
</rule>
<rule ref="PSR2.Namespaces.NamespaceDeclaration"/>
<rule ref="PSR2.Namespaces.UseDeclaration">
<exclude name="PSR2.Namespaces.UseDeclaration.UseAfterNamespace"/>
<rule ref="SlevomatCodingStandard.Commenting.ForbiddenComments">
<properties>
<property name="forbiddenCommentPatterns" type="array" value="/Class .*/"/>
</properties>
</rule>

<!-- PSR-12 sniffs -->
<rule ref="PSR12.Functions.ReturnTypeDeclaration" />

<!-- SlevomatCodingStandard sniffs -->
<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses" />
<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint" />
<rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint" />
<rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint" />
<rule ref="SlevomatCodingStandard.Commenting.InlineDocCommentDeclaration"/>
<rule ref="SlevomatCodingStandard.Commenting.UselessFunctionDocComment"/>
<rule ref="SlevomatCodingStandard.Commenting.UselessInheritDocComment"/>
Comment on lines +107 to +108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these conflict with Drupal's standards.

Copy link
Collaborator Author

@danepowell danepowell Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses"/>
<rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes"/>
<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint">
<exclude name="SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingTraversableTypeHintSpecification"/>
</rule>
<rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint"/>
<rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint"/>
Comment on lines +117 to +121
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<!-- 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"/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These slevomat standards are some of my faves. If they're too opinionated or just not your jam, let me know.


<!-- Squiz sniffs -->
<rule ref="Squiz.Arrays.ArrayBracketSpacing"/>
Expand Down Expand Up @@ -163,52 +142,6 @@
<rule ref="Squiz.Arrays.ArrayDeclaration.ValueNoNewline">
<severity>0</severity>
</rule>
<rule ref="Squiz.ControlStructures.ForEachLoopDeclaration"/>
<!-- Disable some error messages that we already cover. -->
<rule ref="Squiz.ControlStructures.ForEachLoopDeclaration.AsNotLower">
<severity>0</severity>
</rule>
<rule ref="Squiz.ControlStructures.ForEachLoopDeclaration.SpaceAfterOpen">
<severity>0</severity>
</rule>
<rule ref="Squiz.ControlStructures.ForEachLoopDeclaration.SpaceBeforeClose">
<severity>0</severity>
</rule>
<rule ref="Squiz.ControlStructures.ForLoopDeclaration"/>
<!-- Disable some error messages that we already cover. -->
<rule ref="Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterOpen">
<severity>0</severity>
</rule>
<rule ref="Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeClose">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration"/>
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.BraceOnSameLine">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.ContentAfterBrace">
<severity>0</severity>
</rule>
<!-- Standard yet to be finalized on this
(https://www.drupal.org/node/1539712). -->
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.FirstParamSpacing">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.Indent">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.CloseBracketLine">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing">
<properties>
<property name="equalsSpacing" value="1"/>
</properties>
</rule>
<rule
ref="Squiz.Functions.FunctionDeclarationArgumentSpacing.NoSpaceBeforeArg">
<severity>0</severity>
</rule>
<rule ref="Squiz.PHP.LowercasePHPFunctions"/>
<rule ref="Squiz.Strings.ConcatenationSpacing">
<properties>
Expand All @@ -222,11 +155,8 @@
</properties>
</rule>
<rule ref="Squiz.WhiteSpace.LanguageConstructSpacing"/>
<rule ref="Squiz.WhiteSpace.OperatorSpacing" />
<rule ref="Squiz.WhiteSpace.OperatorSpacing"/>
<rule ref="Squiz.WhiteSpace.SemicolonSpacing"/>
<rule ref="Squiz.WhiteSpace.SuperfluousWhitespace"/>

<!-- Zend sniffs -->
<rule ref="Zend.Files.ClosingTag"/>

</ruleset>
Loading