Add PhpStan (PHP Static Code Analyser) PreCommit Hook #545

taufek · 2018-01-28T09:06:55Z

Added PhpStan PreCommit Hook.

Example Files Change

// app/foo.php
<?php

namespace App;

class Foo
{
    public function __construct($foo, $test)
    {
        $this->foo = $foo;
    }
}

// app/bar.php
<?php

namespace App;

class Bar
{
    public function __construct($foo)
    {
        $this->foo = $foo;
    }

    private function testMe()
    {
        return $this->hello;
    }
}

Example Error Messages

Running pre-commit hooks
Analysing PHP codes...............................[PhpStan] FAILED
Errors on modified lines:
/app/bar.php:9:Access to an undefined property App\Bar::$foo.
/app/bar.php:14:Access to an undefined property App\Bar::$hello.
/app/foo.php:7:Constructor of class App\Foo has an unused parameter $test.
/app/foo.php:9:Access to an undefined property App\Foo::$foo.

✗ One or more pre-commit hooks failed

sds

Thanks for the PR, @taufek. A few minor issues with the implementation but overall looks good. Fix those (and rebase) and we'll be good to merge here. Thanks!

sds · 2018-01-30T00:45:16Z

spec/overcommit/hook/pre_commit/php_stan_spec.rb

+    before do
+      sample_output = [
+        ''
+      ].join("\n")


This might be better expressed as just an empty string.

sample_output = ''

sds · 2018-01-30T00:46:51Z

spec/overcommit/hook/pre_commit/php_stan_spec.rb

+          '/sample1.php:14:Call to an undefined static method Sample1::where()',
+          '/sample2.php:17:Anonymous function has an unused use $myVariable.'
+        ].join("\n")
+        # rubocop:enable Metrics/LineLength


Why is this comment here? It doesn't seem to be necessary.

Opps. Will remove it.

sds · 2018-01-30T00:48:32Z

config/default.yml

    include: '**/*.php'

+  PhpStan:
+    description: 'Analysing PHP codes'


Let's keep this consistent with what we do elsewhere and write:

Analyze with phpstan

sds · 2018-01-30T00:53:53Z

lib/overcommit/hook/pre_commit/php_stan.rb

+      applicable_files.each do |file|
+        result = execute(command, args: [file])
+        output = result.stdout.chomp
+        if result.status


status is a number, which will always be truthy, which I don't think is what you intended here. Let's use

result.success?

Thanks. Will update.

sds · 2018-01-30T00:54:32Z

lib/overcommit/hook/pre_commit/php_stan.rb

+      messages = []
+
+      applicable_files.each do |file|
+        result = execute(command, args: [file])


Is there a reason you're only running one file at a time? This will run very slowly for a large number of files, since you're running them in serial.

Unless there's something about how the tool manages this, would strongly recommend you pass all files to phpstan in one run.

Will update. It does make sense.

sds · 2018-01-30T00:55:47Z

lib/overcommit/hook/pre_commit/php_stan.rb

+
+      applicable_files.each do |file|
+        result = execute(command, args: [file])
+        output = result.stdout.chomp


You don't need to assign this variable unless the exit status was successful, so perhaps only do this if result.success?

I'll give it a try.

Added [PhpStan](https://github.com/phpstan/phpstan) PreCommit Hook. **Example Files Change** ``` // app/foo.php <?php namespace App; class Foo { public function __construct($foo, $test) { $this->foo = $foo; } } // app/bar.php <?php namespace App; class Bar { public function __construct($foo) { $this->foo = $foo; } private function testMe() { return $this->hello; } } ``` **Example Error Messages*** ``` Running pre-commit hooks Analysing PHP codes...............................[PhpStan] FAILED Errors on modified lines: /app/bar.php:9:Access to an undefined property App\Bar::$foo. /app/bar.php:14:Access to an undefined property App\Bar::$hello. /app/foo.php:7:Constructor of class App\Foo has an unused parameter $test. /app/foo.php:9:Access to an undefined property App\Foo::$foo. ✗ One or more pre-commit hooks failed ```

taufek · 2018-01-30T01:34:17Z

@sds ,

Thanks for the feedback. I've updated accordingly. Ready for re-review.

sds · 2018-01-30T07:47:31Z

Thanks for addressing those comments, @taufek. Looks great!

taufek force-pushed the tj-phpstan branch 3 times, most recently from 9e16644 to 8fdcc24 Compare January 29, 2018 15:26

sds reviewed Jan 30, 2018

View reviewed changes

taufek force-pushed the tj-phpstan branch from 8fdcc24 to cae64c6 Compare January 30, 2018 01:33

sds merged commit e29265e into sds:master Jan 30, 2018

sds added the enhancement label Jan 30, 2018

taufek deleted the tj-phpstan branch February 7, 2018 11:29

Add PhpStan (PHP Static Code Analyser) PreCommit Hook #545

Add PhpStan (PHP Static Code Analyser) PreCommit Hook #545

Uh oh!

Conversation

taufek commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taufek commented Jan 30, 2018

Uh oh!

sds commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

taufek commented Jan 28, 2018 •

edited

Loading