Add Quality assurance tools to a legacy project

The story

It’s often difficult to make others agree on removing unused code, enhance code quality or simply add tooling or methodology.Here’s a adapted story from a real case.

I joined a project which I had to maintain. There were different people who worked on it. Without maintained unit tests, code style processor or static analyser.They didn’t use a lot of type hinting or update PHPDoc on a PHP 7.1 project. It was total chaos. 😱

I found it really hard to debug, trace errors and to add features without breaking everything.Then, I first tried to rewrite a lot of things, added PHPStan , more PHPUnit tests , Behat scenarios and opened a PR. It wasn’t welcomed and closed directly.

Other devs here didn’t want to make the code better for the current version but only wanted to negociate a total rewrite of the app.So my proposal wasn’t helpful to them as it would prove it could be maintain and enhance.

But everyone in the company knows that management would never have wanted a rewrite. The project still works. One thing they, as manager, don’t actually see is:They lose a lot of money everytime someone spend time debugging manually, testing manually and everyting.Another thing the team doesn’t want to understand is that they would spend too much time and make their company lose a lot of money too if they would go to the full rewrite path.

Then, from my point of view , the best way to get both parties to agree was to incrementally update.This time not by hand! There is a bunch of tools out there developed in Open Source and we’ve got a Gitlab CI environment ready.It was time to use it to prove what was needed to improve, rewrite, delete.

For the management team, you need numbers , cost informations , but most importantly 💲.

• How much would it cost? 💸
• How much will we earn? 💰
• How much time will you need? ⏲

There is already some good lists for QA so I will not be exhaustive but only talk about the current tools i’m working with.

Add tools through composer

We will use different types of tools:

• Coding Standards
• Static Analysis
• A Copy/Paste Detector (CPD) for PHP code.
• A mess detector
• A tool for quickly measuring the size of a PHP project.

Add them to composer:

$ composer require --dev sylius-labs/coding-standard phpstan/phpstan sebastian/phpcpd phpmd/phpmd phploc/phploc

Then add scripts to your composer.json:

{
    "scripts": {
            "code-style": [
                "vendor/bin/ecs check $(git diff --name-only --diff-filter=ACMRTUXB HEAD~..HEAD -- '*.php')"
            ],
            "code-style-fix": [
                "@code-style --fix"
            ],
            "static-analysis": [
                "vendor/bin/phpstan analyse $(git diff --name-only --diff-filter=ACMRTUXB HEAD~..HEAD -- '*.php') -c phpstan.neon"
            ],
            "mess-detector": [
                "vendor/bin/phpmd $(git diff --numstat --name-only --diff-filter=ACMRTUXB HEAD~..HEAD -- '*.php' | awk -vORS=, '{ print $1 }' | sed 's/,$//') text unusedcode,cleancode,controversial,design"
            ]
    }
}

Start with examples

In each of the next commits, you will use those scripts to make adjustments.Thanks to git diff --name-only --diff-filter=ACMRTUXB HEAD~..HEAD -- '*.php you will only see errors for files youchanged.

Work as usual and run commands:

$ composer code-style

$ composer code-style-fix

$ composer static-analysis -- --level max # start from 0 then upgrade the level toward 8.

You will only make small changes. Your team will not be able to disagree to thoses improvements.They will see benefits in using your new tools.

Beware of one thing when using the EasyCodingStandard rules from Sylius:It will add declare(strict_types=1); to your files.

You can remove this rule easily:

imports:
  - { resource: 'vendor/sylius-labs/coding-standard/easy-coding-standard.yml' }

parameters:
  skip:
    # Will need to be remove when the entire codebase will be fully fixed with phpstan and ecs
    # @see https://stackoverflow.com/questions/48723637/what-does-strict-types-do-in-php
    PhpCsFixer\Fixer\Strict\DeclareStrictTypesFixer:
      - '*.php'

Involve your colleagues

Add Continues integration

Then, you need to make your team aware of what’s wrong in the project and in each of they’re commits.The only condition that’s really herd to enforce is YOU CAN’T MERGE IF THE PIPELINE FAILS.It would be easier if they are impressed by the previous improvements.

People won’t change their habits without being forced to.

Here’s a subset of a Gitlab CI file. As you can see, some steps allow failures.This is what I wanted to say when I talked about incremental updates.

codequality:
  stage: pre-tests
  allow_failure: false
  script:
    - composer code-style
    - composer static-analysis -- --level max --error-format gitlab > phpstan-report.json
  artifacts:
    when: always
    reports:
      codequality: phpstan-report.json
    expire_in: 1 week

mess-detector:
  stage: pre-tests
  allow_failure: true
  script:
     - vendor/bin/phpmd src html unusedcode,cleancode,controversial,design --reportfile=phpmd.html --ignore-violations-on-exit
  artifacts:
    paths:
      - phpmd.html
    expire_in: 1 week

copy-paste-detector:
  stage: pre-tests
  allow_failure: true
  script:
     - vendor/bin/phpcpd src --fuzzy --log-pmd=phpcpd.xml || true # there is no flag to ignore violations on exit
  artifacts:
    paths:
      - phpcpd.xml
    expire_in: 1 week

phploc:
  stage: pre-tests
  allow_failure: true
  script:
     - vendor/bin/phploc src --log-xml=phploc.xml
  artifacts:
    paths:
      - phploc.xml
    expire_in: 1 week

Add code review

After a ticket has been made, every change should be read and discussed with at least one fellow developer.This exchange would allow to help each other, validate that the code corresponds to business expectations and that the tests don’t seem to have any flaws.

What will be your workflow

From now on, you will have the informations to know what’s wrong.Locally, while working, use the composer scripts we’ve added to make adjustments. PHPStan and a Code Style fixer will be your best friends.Through the CI, you will enforce your new rules.

In fine, you will remove superfluous line of code, useless classes, add more types, make the codebase easier to work with.Don’t forget to delete unused classes, methods and variables.

Commented lines of code are also potential errors

One more thing, you can also remove all of the commented lines of code if you’re using a version control system like Git.

Create a new branch. From there, remove all the CLOC on all the project.They are useless piece of information. Why? Because it hides what’s important.

Finally, if you really wanted to rewrite your project, it will be easier from there.All that remains is to choose metrics to evaluate the achievement of the approach.They can take the form of a minimum tests coverage on critical points of an application, for example.

Even if your project is not bloated with a lot of bugs, if you’re just started your project, start with the right methodology!I didn’t talk about tests in this blog post but they are really the best way to have a good quality product. They areway more important to add to your workflow than static analysis.

To me, people who are not working with those tools or equivalent and not doing tests on their projects are not professionals, they are hobbyists. 🤜 🖐️ 🎤