add phpstan check to ci

[brambaud]

See issue #88

Here's a first try to integrate phpstan :)

With the higher phpstan level (the higher the stricter), i.e. 7, there is at this moment 221 errors detected.

Not all of them are relevant IMHO.
For instance there is a lot of "errors" like Return typehint of method KnpU\OAuth2ClientBundle\Client\Provider\SlackClient::fetchUserFromToken() has invalid type AdamPaterson\OAuth2\Client\Provider\SlackResourceOwner. that could be ignore.

I'd like to have opinion of others about how to proceed about the detected errors.
Do we handle them in this PR?
Which errors should be ignored?
Should we decrease the phpstan level at first and increase it little by little?

[bocharsky-bw]

Hm, somethings is wrong, I don't see TravisCI was executed on this PR :/

[brambaud]

Me neither... Is there any issue with the modifications on the .travis.yml files? 🤔

[bocharsky-bw]

Hm, try to add a new empty commit, e.g. git commit --allow-empty and push again, probably some temporary outage of TravisCI outage. Not sure your changes might affect this

[brambaud]

According to https://travis-ci.org/knpuniversity/oauth2-client-bundle/requests it seems that something is wrong with the .travis.yml file

Could not parse knpuniversity/oauth2-client-bundle/.travis.yml@717a3f868299b75f

[brambaud]

travis-ci does not seem to support test command. I add to replace

[ "$LINT" = 1 ] && ./vendor/bin/phpstan analyse

by

if [ "$LINT" = 1 ]; then ./vendor/bin/phpstan analyse; fi;

Did not know that... Too bad... :/

[weaverryan]

Hey @brambaud!

Thanks for the PR :). I would be interested in decreasing the level until there are a reasonable level of failures. And then yes, we would fix them in this PR to get things green. Then we could investigate increasing the level over time if we want.

Cheers!

[brambaud]

The level has been decreased to level 1 :)

The errors like Return typehint of method KnpU\OAuth2ClientBundle\Client\Provider\SlackClient::fetchUserFromToken() has invalid type AdamPaterson\OAuth2\Client\Provider\SlackResourceOwner. are ignored.

We have 3 remains errors:

 ------ ------------------------------------------------------------------ 
  Line   DependencyInjection/Providers/GeocachingProviderConfigurator.php  
 ------ ------------------------------------------------------------------ 
  32     Class League\OAuth2\Client\Provider\Geocaching not found.         
 ------ ------------------------------------------------------------------ 
 ------ ----------------------------------------------------------------- 
  Line   DependencyInjection/Providers/InstagramProviderConfigurator.php  
 ------ ----------------------------------------------------------------- 
  26     Class League\OAuth2\Client\Provider\Instagram not found.         
 ------ ----------------------------------------------------------------- 
 ------ ------------------------------------------------------------ 
  Line   DependencyInjection/Providers/JiraProviderConfigurator.php  
 ------ ------------------------------------------------------------ 
  26     Class Mrjoops\OAuth2\Client\Provider\Jira not found.        
 ------ ------------------------------------------------------------ 

IMHO they are false positive. Within the context using ::class keyword to resolve the FQDN is fine even if the class does not exist.
Nevertheless I'm not sure how to handle them. We could ignore all errors like Class [a-zA-Z0-9\\_] not found inside the folder src/DependencyInjection/Providers 🤔

[weaverryan]

Nevertheless I'm not sure how to handle them. We could ignore all errors like Class [a-zA-Z0-9\_] not found inside the folder src/DependencyInjection/Providers

I guess? I've used libraries that have phpstan on them, but have never configured it myself. I'm not sure if there's a better way.

[brambaud]

I'm not sure if there's a better way.

Since the error is very generic the other way I can think of is to write a custom error filter.
Nevertheless I choose to ignore them for the moment and stay simple :)

[weaverryan]

Thanks @brambaud! If someone knows of a better way to ignore these errors (or even a change in the code itself to help), they can let us know.

[alister]

I'd be happy to take a look at these over the weekend. From a quick look, there only looks to be 8 or 9 actual distinct issues.

Most of the 90+ issues with ResourceOwnerInterface is just the @return type causing PHPStan to complain. It's actually a mix of the error where the various ::class are not known yet, because they aren't required dependencies, but then also the comment on #33 about removing | ResourceOwnerInterface. PHPStan can't know what those external classes implement, so must presume it does not. The hard part may be going and looking if it really does provide it.

[alister]

#204 is ready. It looks like some of the recent commits to Symfony/Symfony dev-master also removed the Treebuilder::root, so I've made those checks more robust as well. I'm happy to drop that, or some of the other commits, from the PR, if you think they are a little too strict.

[weaverryan]

It looks like some of the recent commits to Symfony/Symfony dev-master also removed the Treebuilder::root

Ah, I commented about this, but now I understand better. That is by design - the deprecated root() method is removed in master for 5.0. But we should still "reverse" the check (as I commented). I think it should work smoothly the way it was... but maybe I'm missing something?

[alister]

I did take another look at this, and managed to do without the reversal. Take a look and let me know if there is anything else I can do (or, equally, drop) to finish this up.