PHPMD & Code Quality Scans

[aweingarten]

User Story As dev lead I want the build code quality checks to prevent code with complexity from passing.

Background Very few people understand how to write conditional code and functions well. Adding these automated checks will help to enforce some degree of sanity among less experienced developers.

Implementation Approach We are looking to add a PHPMD to scan for cyclomatic complexity and functions that exceed x lines. Is this of interest to people? Has it been attempted before?

Here is a set of rules for PHPMD https://phpmd.org/rules/index.html

Build Failure Criteria

• Cyclomatic Complexity Exceeding X
• functiongs exceeding X lines.
• TBD

cc: @grasmash, @TravisCarden, @ashabed

[TravisCarden]

Hey, @aweingarten! As to attempting it, I'm using PHPMD on my current (internal) project. I run it on Git pre-commit using brainmaestro/composer-git-hooks and on Travis CI builds. It works nicely. As to whether or not anyone would be interested in it (being added to BLT), @ba66e77 and @danepowell are the people to ping about that these days. My only input on the desirability of the tool's inclusion would be to say that by default it sets the bar higher than the average developer will comfortably clear, and it would probably be wise to only run a small subset of the rules or modify the severity weights so that most will emit notices and only a select few will fail the build. (If you need to ease a legacy codebase into compliance, you can run it with --ignore-violations-on-exit to only print the results without throwing an error until you can whittle away the issues.) If included in BLT, I think it should definitely be as an optional feature or a recipe.

[aweingarten]

@TravisCarden figured you were one of the only other people I knew with an opinion on the subject!

I agree a recipe with some liberal defaults makes sense. We can have the recipe generate an ant/xml file with the basics. Or make it optional via config.

@TravisCarden for your project what defaults are you using? Might we steal your config as a baseline?

[dpagini]

I would love to see this make it into BLT, or even if it doesn't, would love to hear more from @TravisCarden how he's using it. Using PHPCS has been an invaluable tool, and to have a tool to enforce even more best practices would be 👌.

[TravisCarden]

@aweingarten / @dpagini, here's an extract from my composer.json which I've modified slightly to better reflect your use case.

    "require-dev": {
        "brainmaestro/composer-git-hooks": "^2.5",
        "phpmd/phpmd": "^2.6"
    },
    "extra": {
        "hooks": {
            "pre-commit": [
                "set -e",
                "vendor/bin/phpmd docroot/modules/custom,docroot/themes/custom text phpmd.xml.dist"
            ],
            "post-install-cmd": "vendor/bin/cghooks add --ignore-lock",
            "post-update-cmd": "vendor/bin/cghooks update"
        }
    },

Here's my phpmd.xml.dist:

<?xml version="1.0" encoding="UTF-8"?>

<!-- https://phpmd.org/documentation/creating-a-ruleset.html -->
<ruleset xmlns="http://pmd.sf.net/ruleset/1.0.0"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://pmd.sf.net/ruleset/1.0.0 http://pmd.sf.net/ruleset_xml_schema.xsd"
         xsi:noNamespaceSchemaLocation="http://pmd.sf.net/ruleset_xml_schema.xsd"
>

  <rule ref="rulesets/cleancode.xml"/>
  <rule ref="rulesets/codesize.xml"/>
  <rule ref="rulesets/design.xml"/>
  <rule ref="rulesets/naming.xml">
    <exclude name="ShortVariable"/>
    <exclude name="ShortMethodName"/>
  </rule>
  <rule ref="rulesets/unusedcode.xml"/>

</ruleset>

I emphasize again that this is what I use to check my code. It's strict! If you apply this as-is to your project expect pull requests to instantly cease.

Now have fun storming the castle! 😉

[danepowell]

I think this sounds like a neat idea, but it's not something we can commit resources to developing further right now. If you can get it out of WIP, and ensure that it's configured in a way that it won't negatively impact existing projects (i.e. too many false positives) then I'd be willing to review it again. Thanks!