magento / magento-coding-standard

Magento Coding Standard
Open Software License 3.0
349 stars 153 forks source link

Using phpro/grumphp for local quality check #35

Closed larsroettig closed 5 years ago

larsroettig commented 5 years ago

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:

mzeis commented 5 years ago

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 commented 5 years ago

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

lenaorobei commented 5 years ago

@larsroettig sure, adding to the meeting notes.

orlangur commented 5 years ago

@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 commented 5 years ago

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

lenaorobei commented 5 years ago

@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?

larsroettig commented 5 years ago

This issue was moved to magento/architecture#90