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

Removed the exclusion of docroot from phpcs. #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sonnykt
Copy link
Contributor

@sonnykt sonnykt commented Oct 21, 2019

Currently ahoy lint and PHPCS do not check any code in docroot/modules/custom due to the exclusion in phpcs.xml. We need to remove this exclusion so that CI can check for coding standard in custom modules.

On-behalf-of: @salsadigitalauorg [email protected]

@sonnykt sonnykt self-assigned this Oct 21, 2019
Copy link
Contributor

@anthony-malkoun anthony-malkoun left a comment

Choose a reason for hiding this comment

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

Does this then also lint a bunch of other directories besides the docroot/modules/custom directory?

@@ -14,7 +14,6 @@
key "config.platform.php".-->
<config name="testVersion" value="7.1"/>

<exclude-pattern>*/docroot/*</exclude-pattern>
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this also then do everything in contrib as part of the lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have PHPCS_TARGETS in .env

@anthony-malkoun
Copy link
Contributor

Also, does composer.json need updating?

@sonnykt sonnykt force-pushed the hotfix/fix_phpcs_docroot branch 3 times, most recently from a68eb67 to 4105d04 Compare October 21, 2019 08:05
@sonnykt sonnykt force-pushed the hotfix/fix_phpcs_docroot branch from 4105d04 to eb7179a Compare October 21, 2019 08:18
@christopher-hopper
Copy link
Contributor

I'm also encountering this problem. I noticed custom code I am reviewing is passing tests, even though it violates Drupal coding standards. Digging into why, I found this phpcs.xml is excluding everything.

Could this phpcs.xml could be brought up-to-date with a more flexible, and inclusive structure. A default could be to scan everything in

    <!-- Include these files -->
    <file>docroot/sites/default</file>
    <file>docroot/modules/custom</file>
    <file>docroot/themes/custom</file>
    <file>tests</file>

Rather than excluding lots and lots of things, a smaller inclusions list could help project teams who use SDP. It would work better with IDEs, and either compliment, or negate the need for, a PHPCS_TARGETS env var.

Also, if we renamed it to phpcs.xml.dist we could allow local overrides for individuals. Similar to how phpunit.xml.dist works.

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

Successfully merging this pull request may close these issues.

3 participants