4.x phpstan #210

SignpostMarv · 2018-01-28T11:40:05Z

resolved Confusing phpstan errors #207
switched from phpstan/phpstan-shim to phpstan/phpstan,
adjusted the composer script to behave nicely on gitbash/travis etc.
bumped up some of the phpstan levels, resolved issues.

a phpdoc block in moontoast/math is holding up level 6: https://github.com/ramsey/moontoast-math/blob/c2792a25df5cad4ff3d760dd37078fc5b6fccc79/src/Moontoast/Math/BigNumber.php#L633 , i.e. BigNumber is being passed in when the docs specify string|int

GrahamCampbell · 2018-01-28T12:59:28Z

switched from phpstan/phpstan-shim to phpstan/phpstan,

why?

mhujer · 2018-01-28T13:04:15Z

I already fixed issues from #207 in #208.

SignpostMarv · 2018-01-28T15:33:11Z

@GrahamCampbell in order to use the config files defined in the phpstan package, hoping to progress towards config.levelmax.neon + phpstan-strict-rules

@mhujer I missed that- shall I rebase onto mhujer:mh-4.x-rebase-fixes ?

ramsey · 2018-01-28T17:10:43Z

I’m still not sure what the point of all this work is. It seems like a lot of rigid overhead with little value. I’m happy to include phpstan if others are able to address problems that arise from it, but if I have to keep scratching my head at its error messages, I will likely remove it.

ramsey · 2018-02-08T18:10:19Z

I just merged #208 into 4.x. Should this be rebased against 4.x before I merge it?

mhujer · 2018-02-09T06:49:44Z

composer.json

-            "phpstan analyse -c phpstan.neon src --level 2 --no-progress",
-            "phpstan analyse -c phpstan-tests.neon tests --level 2 --no-progress"
+            "php vendor/phpstan/phpstan/bin/phpstan analyse -c phpstan.neon src --no-progress",
+            "php vendor/phpstan/phpstan/bin/phpstan analyse -c phpstan-tests.neon tests --no-progress"


Why the change from phpstan to the whole path? It should work the old way.

phpstan uses whichever version of phpstan is found under $PATH
vendor/bin/phpstan would work, except for some windows environments, i.e. git bash
vendor/phpstan/phpstan/bin/phpstan refers specifically to the version required by the composer.json

It should always use the one installed through the Composer, see the docs.

Note: Before executing scripts, Composer's bin-dir is temporarily pushed on top of the PATH environment variable so that binaries of dependencies are easily accessible. In this example no matter if the phpunit binary is actually in vendor/bin/phpunit or bin/phpunit it will be found and executed.

In what situation it did not work as expected?

It should always use the one installed through the Composer, see the docs.

might be an artefact of past experience/earlier development environments. checking now.

yup, I stand corrected - always nice to learn new things :)

Great! I was afraid it is actually broken (I use similar configuration elsewhere)

mhujer · 2018-02-09T06:50:52Z

phpstan.neon

@@ -1,3 +1,5 @@
+includes:
+    - vendor/phpstan/phpstan/conf/config.level5.neon


Why is this better than just passing the level from CLI parameters?

it might've changed recently, but there's no cli parameter for the max level, which currently aliases to -l7, but may change in future.

there's also the phpstan/phpstan-strict-rules ruleset, as well as any custom configurations that may be required in future.

Yes, it's possible to use max in CLI.

You can also use --level max as an alias for the highest level.

well colour me corrected ('ve been using the shorthand cli args) :P

pretty sure that importing rules still stands?

i.e. roll back the config file change until such a time as we've got ramsey/uuid passing --level max & we can pull in strict-rules ?

Yep, other rules/extensions should be imported in the config.

mhujer · 2018-02-09T07:00:39Z

@ramsey this should definitely be rebased before merging it to 4.x to verify that the build still passes

SignpostMarv · 2018-02-13T20:34:31Z

rebasing at the moment, dropping the phpunit configs as they seemed to have no effect on the output. i.e.
a0c6e23

…ast/math, as mentioned in ramsey#210

SignpostMarv · 2018-02-13T22:39:03Z

PHP Fatal error: Uncaught RuntimeException: You need to provide valid cache directory for Go! AOP framework. in /home/travis/build/ramsey/uuid/vendor/goaop/framework/src/Core/AspectKernel.php:208

lolwhut ?

mhujer · 2018-02-14T08:13:52Z

src/Provider/Node/FallbackNodeProvider.php

     */
    public function getNode()
    {
        foreach ($this->nodeProviders as $provider) {
-            if ($node = $provider->getNode()) {
+            if ((bool) ($node = $provider->getNode())) {


What about something like this:

$node = $provider->getNode(); if ($node !== null) { return $node; }

When PHPStan warns you, that there is implicit boolean conversion in condition, it does not mean that it should be replaced with explicit 😄

mhujer · 2018-02-14T08:15:51Z

phpstan.neon

@@ -1,7 +1,10 @@
+includes:
+    - ./vendor/phpstan/phpstan-strict-rules/rules.neon


I would put this (and the necessary changes) in a separate PR.

I think it makes sense to fix the stuff PHPStan detects with the standard checks first. And then consider enabling this. (When doing both at the same time, you can't tell if the strange Travis error is related to this or not)

the changes are relatively minimal for it to pass the strict rules ?

They probably are, so maybe it would be a wasted afford in this case.

Anyway, I just tried to build the 4.x branch itself and it fails as well https://travis-ci.org/mhujer/uuid/jobs/341774850 - so it is probably caused by a new version of https://github.com/goaop/framework/releases

Probably it would be enough to configure cacheDir https://github.com/goaop/framework#3-configure-the-aspect-kernel-in-the-front-controller in tests/bootstrap.php (I would love to see this in a separate PR though ;-) )

I've been giving some thought to separate pull requests on turning the error suppression back off- tinkering with an autoload-dev shim that matches the method signatures of the missing functions & constants, but throws BadFunctionCallException if inadvertently called ?

SignpostMarv · 2018-02-20T22:34:14Z

have rolled back the strict rules commits so we ca put them on a separate pr later

mhujer · 2018-02-21T07:22:08Z

I've created a PR which fixes the build #214

… on Ramsey\Uuid\Converter\TimeConverterInterface::calculateTime()

…y phpstan level 3

…ast/math, as mentioned in ramsey#210

…y RandomLibAdapter::$generator not being type hinted as nullable

SignpostMarv · 2018-02-21T18:35:21Z

@mhujer rebased, seems to be passing now.

ramsey · 2018-07-19T18:00:03Z

Thank you for contributing! 🎉

mhujer reviewed Feb 9, 2018

View reviewed changes

SignpostMarv force-pushed the 4.x--phpstan branch from 0d2f2f1 to ce02517 Compare February 13, 2018 21:09

SignpostMarv added a commit to SignpostMarv/uuid that referenced this pull request Feb 13, 2018

bumping phpstan level, supressing error caused by docblocks on moonto…

dcf16dd

…ast/math, as mentioned in ramsey#210

mhujer reviewed Feb 14, 2018

View reviewed changes

mhujer mentioned this pull request Feb 15, 2018

Tests: configure cacheDir for AspectMock #213

Merged

SignpostMarv force-pushed the 4.x--phpstan branch from ce02517 to 2e45b91 Compare February 20, 2018 22:31

SignpostMarv added 12 commits February 21, 2018 18:10

bumping phpstan level

b9b7369

adjusting docs to match existing tests & return types

4be1dca

bumping phpstan level

d8a1a93

adjusting docs to match existing tests & return types

904a30a

bumping phpstan level

f379939

typecasting to resolve phpstan issue triggered by return type defined…

5fafb5f

… on Ramsey\Uuid\Converter\TimeConverterInterface::calculateTime()

bumping phpstan level

e803ec4

calling unset rather than setting null to resolve issues identified b…

569f58f

…y phpstan level 3

bumping phpstan level, no issues identified

34f4904

tests pass so we can probably safely supress this error

deb3a89

bumping phpstan level, supressing error caused by docblocks on moonto…

b4fb954

…ast/math, as mentioned in ramsey#210

bumping phpstan level, swapping order of property assignment caused b…

17f954d

…y RandomLibAdapter::$generator not being type hinted as nullable

setting level to max

4643624

SignpostMarv force-pushed the 4.x--phpstan branch from 2e45b91 to 4643624 Compare February 21, 2018 18:11

ramsey merged commit ae67e4c into ramsey:4.x Jul 19, 2018

SignpostMarv deleted the 4.x--phpstan branch July 19, 2018 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4.x phpstan #210

4.x phpstan #210

SignpostMarv commented Jan 28, 2018

GrahamCampbell commented Jan 28, 2018

mhujer commented Jan 28, 2018

SignpostMarv commented Jan 28, 2018

ramsey commented Jan 28, 2018

ramsey commented Feb 8, 2018

mhujer Feb 9, 2018

SignpostMarv Feb 9, 2018

mhujer Feb 10, 2018

SignpostMarv Feb 10, 2018

SignpostMarv Feb 10, 2018

mhujer Feb 10, 2018

mhujer Feb 9, 2018

SignpostMarv Feb 9, 2018

mhujer Feb 10, 2018

SignpostMarv Feb 10, 2018 •

edited

mhujer Feb 10, 2018

mhujer commented Feb 9, 2018

SignpostMarv commented Feb 13, 2018 •

edited

SignpostMarv commented Feb 13, 2018 •

edited

mhujer Feb 14, 2018

mhujer Feb 14, 2018

SignpostMarv Feb 14, 2018

mhujer Feb 15, 2018 •

edited

mhujer Feb 15, 2018

SignpostMarv Feb 20, 2018

SignpostMarv commented Feb 20, 2018

mhujer commented Feb 21, 2018

SignpostMarv commented Feb 21, 2018

ramsey commented Jul 19, 2018

		@@ -1,3 +1,5 @@
		includes:
		- vendor/phpstan/phpstan/conf/config.level5.neon

		@@ -1,7 +1,10 @@
		includes:
		- ./vendor/phpstan/phpstan-strict-rules/rules.neon

4.x phpstan #210

4.x phpstan #210

Conversation

SignpostMarv commented Jan 28, 2018

GrahamCampbell commented Jan 28, 2018

mhujer commented Jan 28, 2018

SignpostMarv commented Jan 28, 2018

ramsey commented Jan 28, 2018

ramsey commented Feb 8, 2018

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

Choose a reason for hiding this comment

SignpostMarv Feb 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhujer commented Feb 9, 2018

SignpostMarv commented Feb 13, 2018 • edited

SignpostMarv commented Feb 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhujer Feb 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SignpostMarv commented Feb 20, 2018

mhujer commented Feb 21, 2018

SignpostMarv commented Feb 21, 2018

ramsey commented Jul 19, 2018

SignpostMarv Feb 10, 2018 •

edited

SignpostMarv commented Feb 13, 2018 •

edited

SignpostMarv commented Feb 13, 2018 •

edited

mhujer Feb 15, 2018 •

edited