N°8796 - Add pre-commit hooks for auto PHP code style fixing

[Molkobain]

Base information

Question	Answer
Related to a SourceForge thead / Another PR / Combodo ticket?	N°8796
Type of change?	Code quality

Objective (enhancement)

In a previous PR, we introduced the PHP code style tool (PHP CS Fixer), a re-formatting of the code base and a comformity check in the CI.

This PR aims at easing developer's experience by automatically running the PHP code style fixer on files about to be committed. Avoiding to manually launch the tool or to make a "Fix CI" commit after pushing non-comform code.

Proposed solution

What has been done:

• Add a Git hooks manager running with PHP (CaptainHook)
• Run PHP-CS-Fixer on pre-commit hook to fix code style of staged PHP files
  • Why use a custom PHP script instead of the "native" PHP-CS-Fixer command in the <ITOP>/captainhook.json?
    • When running PHP-CS-Fixer native command on the pre-commit hook it would either:
      • In case of check, block the commit, which was good but forced the developer to manually run PHP-CS-Fixer, then re-commit => Not OK
      • In case of fix, commit files anyway, then fix them for a second commit => Not OK
    • Using a custom PHP script, we can run the fix command on files, then re-stage them for the current commit, which makes it a transparent operation for the developer

How to use it:

• Install the PHP code style tool (PHP-CS-Fixer), must be done only for the 1st run
  • Go to <ITOP>/tests/php-code-style folder
  • Install the dependencies composer install
• Install the Git hooks manager (CaptainHook), must be done only for the 1st run
  • Go to <ITOP> folder
  • Duplicate <ITOP>/captainhook.config.json.dist into <ITOP>/captainhook.config.json
  • Edit it to keep only 1 php-path line and adjust it to your environement (It's important to change this BEFORE running the next command, otherwise the Git hook will be installed with an incorrect path to PHP)
  • Install the dependencies AND the git hooks on your environment composer install
• Make some changes on PHP files
• Commit these PHP files
• In the Git history, you should see that these files were modified to match PHP code style if they didn't already

Checklist before requesting a review

• I have performed a self-review of my code
• I have tested all changes I made on an iTop instance
• I have added a unit test, otherwise I have explained why I couldn't => No test coverage on this as it is not included in the package tested by the CI.
• Is the PR clear and detailed enough so anyone can understand digging in the code?

Things to keep in mind

• The Git hook manager has been added to Composer through require-dev just like for some other dependencies (e.g. "web profiler"), it should be up to the Factory to clean these "dev dependencies" when building the final package.
• Maybe we should update the README.md or add a developers section to explain how to set-up a development environment.

[steffunky]

Works on Linux with a PHP not located at default location

[bdalsass]

Work with project on WSL with PHP 8.3.

Just getting somme warning messages on composer install:

Wondering if the captainhook library could be install globally to avoid installing it on each project....

[Molkobain]

This won't be done that way. Instead we choose to configure PHPStorm for each projet so it reformats code in all repositories, not only the iTop one.

[hakonharnes]

This won't be done that way. Instead we choose to configure PHPStorm for each projet so it reformats code in all repositories, not only the iTop one.

The contributing guide asks external contributors to follow the PHP coding standards but offers no local tooling to help. Relying on PHPStorm configuration won't help contributors using other editors. They'll only find out about style issues when CI fails, leading to the exact "fix style" commits this PR was trying to avoid.

I don't think the two approaches are mutually exclusive. The PHPStorm config for the internal team and pre-commit hooks as an IDE-agnostic safety net could co-exist. Might be worth reconsidering, at least as an optional setup for non-PHPStorm users.

[Molkobain]

The contributing guide asks external contributors to follow the PHP coding standards but offers no local tooling to help. Relying on PHPStorm configuration won't help contributors using other editors. They'll only find out about style issues when CI fails, leading to the exact "fix style" commits this PR was trying to avoid.

I don't think the two approaches are mutually exclusive. The PHPStorm config for the internal team and pre-commit hooks as an IDE-agnostic safety net could co-exist. Might be worth reconsidering, at least as an optional setup for non-PHPStorm users.

Thanks for pointing this out @hakonharnes ! I don't think we'll keep the pre-commit approach as it required to include unnecessary third-party libs.

But we can update the contributing guide to point to both static analysis and code style READMEs tools as they contain the instructions to run both tools locally.

What do you think?

[hakonharnes]

I think that'd be a logical improvement :)

[Molkobain]

Done, I've added a "Developers" section in the main README.md :)