Skip to content

Using phpro/grumphp for local quality check #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
larsroettig opened this issue Feb 7, 2019 · 7 comments
Closed

Using phpro/grumphp for local quality check #35

larsroettig opened this issue Feb 7, 2019 · 7 comments
Assignees
Labels
need to discuss Rule requires discussion proposal New rule proposal waiting for feedback Additional explanation needed

Comments

@larsroettig
Copy link
Member

larsroettig commented Feb 7, 2019

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
@larsroettig larsroettig added proposal New rule proposal need to discuss Rule requires discussion labels Feb 7, 2019
@mzeis
Copy link
Contributor

mzeis commented Feb 9, 2019

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
Copy link
Member Author

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

@lenaorobei
Copy link
Contributor

@larsroettig sure, adding to the meeting notes.

@orlangur
Copy link

@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
Copy link
Member Author

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

@lenaorobei lenaorobei added the waiting for feedback Additional explanation needed label Feb 13, 2019
@lenaorobei
Copy link
Contributor

@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
Copy link
Member Author

This issue was moved to magento/architecture#90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need to discuss Rule requires discussion proposal New rule proposal waiting for feedback Additional explanation needed
Projects
None yet
Development

No branches or pull requests

4 participants