Using phpro/grumphp for local quality check

[larsroettig]

From @larsroettig on February 7, 2019 15:21

We should add as grumphp for checking the quality of commits.

Sample Config:

parameters:
    bin_dir: "./vendor/bin"
    git_dir: "."
    hooks_dir: ~
    hooks_preset: local
    stop_on_failure: false
    ignore_unstaged_changes: false
    ascii:
        succeeded: ~
        failed: ~
    tasks:
        phpversion:
            project: '7.1'
        xmllint:
            ignore_patterns: ["src/dev"]
        yamllint:
            ignore_patterns: ["src/dev"]
        securitychecker:
            lockfile: ./src/composer.lock
            format: ~
            end_point: ~
            timeout: ~
            run_always: false
        git_blacklist:
            keywords:
            - "die("
            - "var_dump("
            - "print_r("
            - "var_export("
            - "exit;"
            whitelist_patterns:
            - /^src\/app\/(.*)/
            triggered_by: ['php']
        phpmnd:
            directory: ./src/app/code
            whitelist_patterns: []
            exclude: []
            exclude_name: []
            exclude_path: []
            extensions: []
            hint: false
            ignore_numbers: []
            ignore_strings: []
            strings: false
            triggered_by: ['php']
        phpcs:
            standard: "./src/dev/tests/static/framework/Magento/ruleset.xml"
            severity: ~
            error_severity: ~
            warning_severity: 6
            tab_width: ~
            report: summary
            report_width: ~
            whitelist_patterns:
            - /^src\/app\/code\/(.*)/
            encoding: ~
            ignore_patterns: []
            sniffs: []
            triggered_by: [php]
        phpunit:
            config_file: ./dev/tests/phpunit-no-coverage.xml
            testsuite: ~
            group: []
            always_execute: false

We using this tool for more than one year in our projects. We getting better commits and lower rejection rate of Pull Requests.

Info: https://www.integer-net.com/magento-2-automatic-code-quality-check-with-grumphp/

To discuss:

• what should checked before commit
• if can use this tool for only this repo

Copied from original issue: magento/magento-coding-standard#35

[larsroettig]

From @mzeis on February 9, 2019 9:25

I can confirm that GrumPHP also helped us with getting better commits. Especially it's motivating because it shortens the feedback loop (you don't need to wait for a failing build to get feedback) and helps to not forgetting basic checks.

* what should checked before commit

I didn't use phpmnd yet so I cannot tell and we might have to skip the phpversion check if this repository support PHP 5.6 (unless we want to specify that). For the other checks I'm sure they would provide value.

if can use this tool for only this repo

It might be a good test project for Magento to find out if it's suitable for other repos too.

[larsroettig]

@lenaorobei can you review this we should add this to architecture meeting on Wednesday.

[larsroettig]

From @lenaorobei on February 11, 2019 17:9

@larsroettig sure, adding to the meeting notes.

[larsroettig]

From @orlangur on February 13, 2019 16:13

@larsroettig nice initiative, it was a suggested replacement for abandoned composer package used to implement git hook previously :)

I would revisit config a lot thought, git_blacklist for instance could be replaced with ForbiddenFunctions sniff, whitelist_patterns: - /^src\/app\/(.*)/ seems incomplete, phtml files must be checked too.

[larsroettig]

@orlangur ForbiddenFunctions we can add this and remove git_blacklist it is only a recommendation.

[larsroettig]

From @lenaorobei on February 13, 2019 17:28

@larsroettig as per architects discussion this issue looks like more global question than just the coding standard. Could you please move this proposal to magento/architecture repository?

[melnikovi]

Thank you for the proposal @larsroettig .

"We using this tool for more than one year in our projects. We getting better commits and lower rejection rate of Pull Requests" - are PRs being rejected by EngCom team? Does it happen before you get feedback from CI?

These checks run by CI after you push changes. Do you need to manually add pre-commit hook? Not sure many developers would do that. We also would have to maintain this config.

I don't have strong opinion. I suggest to ask in appdesign slack channel and proceed with it if there is enough interest.

[orlangur]

@melnikovi this tool supports multiple use cases, local hooks are just to get quicker response and to maintain quality we can add https://github.com/phpro/grumphp/blob/master/doc/commands.md#run on Travis CI.

[buskamuza]

PHP is required on host (or wherever a dev makes commit from). Most likely all devs should have PHP on host.

[buskamuza]

Let's also run it through developers when a branch with exact rules is ready. @lenaorobei , would you share it with internal teams and find a few volunteers to try it out? @larsroettig , can you discuss it with the community, and find volunteers to test it there?

[buskamuza]

I'd also suggest to add an explicit criteria of "not more than X sec to run" to make sure it doesn't annoy developers. 5s can be a good number or validate with the volunteers.

[lenaorobei]

@buskamuza sure, will do that.

My only one concern is the installation. As I see once we run composer require --dev phpro/grumphp it will invoke grum's Composer post-install/update scripts. Then when somebody will run composer install on Magento, nothing will happen because Composer do not execute scripts from dependencies. Do we need to add post-install-cmd and post-update-cmd to Magento or I'm missing something?

[buskamuza]

Please continue any discussion in https://github.com/magento/architecture/pull/92