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

chore: update phpcs.xml from Drupal examples #670

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hussainweb
Copy link
Member

@hussainweb hussainweb commented Nov 12, 2024

This will lead to many files needing to be recorrected.

Base automatically changed from fix-grumphp-config to main November 12, 2024 00:47
This will lead to many files needing to be recorrected.
@hussainweb hussainweb force-pushed the chore-update-phpcs-xml-from-drupal-examples-this-will-lead-to-many-files-needing-to-be-recorrected branch from 1c465d7 to 4d68e5e Compare November 12, 2024 00:47
Copy link
Collaborator

@zeshanziya zeshanziya left a comment

Choose a reason for hiding this comment

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

@hussainweb I have added my comments. Please review.

<exclude-pattern>*/bower_components/*</exclude-pattern>
<exclude-pattern>*/node_modules/*</exclude-pattern>
<!--Exclude third party code.-->
<exclude-pattern>./assets/vendor/*</exclude-pattern>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hussainweb Exclude is mostly for use when we run phpcs directly. but again, if we don't pass include directories, it will run on whole codebase so if we plan to keep this up and allow user to run phpcs -v then we will have to exclude other directories as well and I think, this will be overkill. We can remove this as we have exclude dirs specified in grumphp conf file.

Mostly exclude all these dirs

<!-- Exclude core, vendor, and contrib directories -->
  <exclude-pattern>./web/core/*</exclude-pattern>
  <exclude-pattern>./vendor/*</exclude-pattern>
  <exclude-pattern>./web/modules/contrib/*</exclude-pattern>
  <exclude-pattern>./web/hemes/contrib/*</exclude-pattern>
  <exclude-pattern>./web/profiles/contrib/*</exclude-pattern>
  <exclude-pattern>./private/*</exclude-pattern>
  <exclude-pattern>./config/*</exclude-pattern>
  <exclude-pattern>./web/sites/*</exclude-pattern>
  <exclude-pattern>./.ddev/*</exclude-pattern>


<!--Allow global variables in settings file.-->
<rule ref="DrupalPractice.CodeAnalysis.VariableAnalysis.UndefinedVariable">
<exclude-pattern>settings(\.[a-zA-Z0-9]+)?\.php</exclude-pattern>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we remove this, phpcs will stop catching UndefinedVariable. Currently it did not catch it in settings.php because /web/sites/default is excluded in grumphp. So if don't want that exclude pattern, we should only keep the rule without any exclude as settings file are already excluded in grumphp.

<rule ref="DrupalPractice.CodeAnalysis.VariableAnalysis.UndefinedVariable" />

Same applies for other DrupalPractice.Commenting rules.

<rule ref="Drupal.Arrays.Array">
<!-- Sniff for these errors: CommaLastItem -->
<exclude name="Drupal.Arrays.Array.ArrayClosingIndentation"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These rules exclude were present earlier too. I removed it. It improves code readability when arrays are properly indented. So I will suggest to remove it.

<exclude name="Drupal.Arrays.Array.LongLineDeclaration"/>
</rule>
<rule ref="Drupal.Classes.ClassCreateInstance"/>
<rule ref="Drupal.Classes.ClassDeclaration"/>
<rule ref="Drupal.Classes.FullyQualifiedNamespace"/>
<rule ref="Drupal.Classes.InterfaceName"/>
<rule ref="Drupal.Classes.UnusedUseStatement"/>
<rule ref="Drupal.Classes.UseGlobalClass"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason for removing this? This checks the following.

You cannot use the use statement for non-namespaced classes; instead, you must reference them with a leading \, e.g., \DateTimeImmutable instead of use DateTimeImmutable;.

<properties>
<property name="ignoreNewlines" value="true"/>
</properties>
</rule>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This checks operator spacing rules.

Here are few examples.

Correct: $a = $b + $c;
Incorrect: $a=$b+$c;

Correct

  $abc = [
    "abc3" => "dssdsd3",
    "abc41" => [
      "dssdsd4"
    ],
  ];

  $abc = [
    "abc3" => "dssdsd3",
    "abc41" => 
    [
      "dssdsd4"
    ],
  ];

Incorrect

$abc = [
    "abc3"=> "dssdsd3",
    "abc41" =>[
      "dssdsd4"
    ],
  ];


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

<!-- sniffs from https://github.com/acquia/coding-standards-php -->
<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are from Acquia coding standards and sorts use statements in ascending order. This can be removed as sorting manually is a challenge but phpcbf can help sort it.


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

<!-- sniffs from https://github.com/acquia/coding-standards-php -->
<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses" />
<rule ref="SlevomatCodingStandard.Variables.DisallowSuperGlobalVariable" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also keep this to discourage direct use of $_GET, $_POST, $_SERVER and $GLOBALS etc.

<!-- Drupal sniffs -->
<rule ref="Drupal"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Spacing in doc comment can be achieved using <rule ref="Drupal.Commenting.DocCommentAlignment"/>.

@hussainweb
Copy link
Member Author

@zeshanziya, can you please revert the changes as you see fit? I didn't make a deliberate effort towards these changes. I only synced it from the latest version of the example phpcs.xml.

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 this pull request may close these issues.

2 participants