Integrating PHPStan & fixes of found issues #842

ondrejmirtes · 2017-01-08T20:55:10Z

This is a PR for integrating static analysis tool PHPStan that focuses on finding bugs and running in your CI build. It found a few issues that I put in separate commits.

lucasmichot · 2017-01-13T09:56:06Z

Hey @ondrejmirtes thanks for this PR (and also for this great PHPStan tool, that use on many of my other projects)
I am not totally sure that it make sense to break a build if unit tests pass though, or to integrate it in TravisCI

Nevertheless the fix are totally valuable

Thoughts on that @briannesbitt ?

ondrejmirtes · 2017-01-13T10:02:39Z

If unit tests pass there's still something that can be broken in your code that isn't covered by tests. Take for example the Translator fixes - if static::translator() returned an instance that doesn't have the addResource method, the code would break. PHPStan enforces more type safety and as a result, you can even write less tests - you don't have to write tests for "boring code" that just calls other methods. For example, people usually don't write tests for getters and setters because that would be a waste of time. But PHPStan can find a mistake such as a typo in a setter, or point out a wrong typehint. Some teams that use PHPStan even run it before running their unit tests.

ondrejmirtes · 2017-01-13T10:04:28Z

Also, PHPStan can find more errors during refactoring that you'd otherwise find during testing or even in production.

lucasmichot · 2017-01-13T10:22:28Z

phpstan.neon

@@ -0,0 +1,7 @@
+parameters:


Can you add this file not to be exported in .gitattributes too?

lucasmichot · 2017-01-13T10:23:03Z

phpstan.neon

@@ -0,0 +1,7 @@
+parameters:
+	ignoreErrors:
+		- '#Access to an undefined property#'


Can you sort error messages ?

lucasmichot · 2017-01-13T10:23:12Z

src/Carbon/Carbon.php

@@ -1091,8 +1091,9 @@ public static function hasRelativeKeywords($time)
    protected static function translator()
    {
        if (static::$translator === null) {
-            static::$translator = new Translator('en');
-            static::$translator->addLoader('array', new ArrayLoader());
+            $translator = new Translator('en');


lucasmichot · 2017-01-13T10:23:33Z

tests/Carbon/ModifyTest.php

@@ -9,6 +9,8 @@
 * file that was distributed with this source code.
 */

+namespace Tests\Carbon;


lucasmichot · 2017-01-13T10:24:24Z

.travis.yml

-script: vendor/bin/phpunit --verbose --coverage-text --coverage-clover=coverage.xml
+script:
+  - vendor/bin/phpunit --verbose --coverage-text --coverage-clover=coverage.xml
+  - if [[ $PHPSTAN = 1 ]]; then composer require --dev phpstan/phpstan:^0.5.2 && vendor/bin/phpstan analyse -c phpstan.neon -l 3 src tests; fi


Can you use long argument notation, config and level ?

Why not level 4?

I will use the long arg notation.

Level 4 is not suitable for Carbon, it's better for PHP 7 codebases with scalar typehints. Take this example:

/** @var bool */ private $foo; /** * @param bool $foo */ public function setFoo($foo) { $this->foo = (bool) $fool }

PHPStan will complain that you're casting bool to bool. But you can't remove the casting because something else can be passed there and to keep the integrity of the object, you need to be sure that only bool is assigned to $this->foo.

But in this case:

public function setFoo(bool $foo) { $this->foo = (bool) $fool }

The (bool) can be safely removed.

Thanks for this explanations @ondrejmirtes , we can stick to level 3 then

ondrejmirtes · 2017-01-13T10:30:20Z

I'll fix your suggestions over the weekend. I'd also like to release 0.6 soon that brings some fixes that will lower the number of ignored errors. Thanks 👍

lucasmichot · 2017-01-15T14:33:20Z

Make sure to rebase and fix the conflict on .travis.yml @ondrejmirtes

…method with `@return $this`

ondrejmirtes · 2017-01-15T22:02:24Z

Fixed, rebased, build passes :)

lucasmichot · 2017-01-16T11:18:48Z

.travis.yml

    - php: 7.1
      env: setup=lowest
    - php: 7.1
      env:
        - coverage=true
        - setup=stable
+    - php: 7.1
+      env: PHPSTAN=1


Can you use phpstan=true (lowercase, pseudo boolean) instead - just to keep consistent ? ;-)

@lucasmichot Updated :)

lucasmichot · 2017-01-16T19:15:50Z

Thanks for the great work @ondrejmirtes !

ondrejmirtes · 2017-01-16T20:38:17Z

@lucasmichot You're welcome 😊 I'd be glad if you considered using PHPStan on your other, maybe even commercial projects 😊 If you have any questions or suggestion for PHPStan, get in touch 😊

ondrejmirtes force-pushed the phpstan branch 4 times, most recently from 9c0284b to 6881bd4 Compare January 8, 2017 21:39

lucasmichot requested a review from briannesbitt January 13, 2017 09:54

lucasmichot reviewed Jan 13, 2017

View reviewed changes

lucasmichot added the WIP label Jan 14, 2017

ondrejmirtes added 5 commits January 15, 2017 19:44

Fixed namespaces of a few tests

f1f47a0

Fixed static call to instance method

97222cb

Fixed method case

07de00f

Fixed translator type handling

9ede617

Consistent @return $this for methods that return result of another …

46cb13e

…method with `@return $this`

ondrejmirtes force-pushed the phpstan branch 4 times, most recently from b61d38c to bffa561 Compare January 15, 2017 20:41

lucasmichot removed the WIP label Jan 16, 2017

lucasmichot reviewed Jan 16, 2017

View reviewed changes

PHPStan integration

113cae1

ondrejmirtes force-pushed the phpstan branch from bffa561 to 113cae1 Compare January 16, 2017 18:58

lucasmichot merged commit a4b1c71 into briannesbitt:master Jan 16, 2017

ondrejmirtes deleted the phpstan branch January 16, 2017 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrating PHPStan & fixes of found issues #842

Integrating PHPStan & fixes of found issues #842

ondrejmirtes commented Jan 8, 2017 •

edited

lucasmichot commented Jan 13, 2017

ondrejmirtes commented Jan 13, 2017

ondrejmirtes commented Jan 13, 2017

lucasmichot Jan 13, 2017

lucasmichot Jan 13, 2017

lucasmichot Jan 13, 2017

lucasmichot Jan 13, 2017

lucasmichot Jan 13, 2017

lucasmichot Jan 14, 2017

ondrejmirtes Jan 14, 2017

lucasmichot Jan 15, 2017

ondrejmirtes commented Jan 13, 2017

lucasmichot commented Jan 15, 2017

ondrejmirtes commented Jan 15, 2017

lucasmichot Jan 16, 2017

ondrejmirtes Jan 16, 2017

lucasmichot commented Jan 16, 2017

ondrejmirtes commented Jan 16, 2017

Integrating PHPStan & fixes of found issues #842

Integrating PHPStan & fixes of found issues #842

Conversation

ondrejmirtes commented Jan 8, 2017 • edited

lucasmichot commented Jan 13, 2017

ondrejmirtes commented Jan 13, 2017

ondrejmirtes commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Jan 13, 2017

lucasmichot commented Jan 15, 2017

ondrejmirtes commented Jan 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmichot commented Jan 16, 2017

ondrejmirtes commented Jan 16, 2017

ondrejmirtes commented Jan 8, 2017 •

edited