Upgrade to PHPStan 0.12

[canvural]

• PHPStan\Analyser\Scope is now an interface. And it's only implementation MutatingScope is marked as @internal So I removed our scope class. It didn't affect any tests. If we find any error, we can try to fix it in another way.

• Currently, the biggest issue is in the OptionsResolver class. We are getting errors like Parameter #1 $option of method _HumbugBox06db9be5fe9f\Symfony\Component\Console\Input\InputDefinition: :addOption() expects _HumbugBox06db9be5fe9f\Symfony\Component\Console\Input\InputOption, Symfony\Component\Console\Input\InputOption given. because PHPStan now distrubuted as PHAR file. (IMO we can drop providing a Laravel specific command and just let the users run PHPStan normally)

• ModelTypeHelper and RelationCreateExtension classes still needs some work.

Closes #381

[nunomaduro]

This is a work in progress or is already stable?

[canvural]

Like I wrote, there are still some errors to solve. Need some ideas for how to solve them.

[nunomaduro]

Thanks!

[canvural]

Another error: analyzing tests/Features/Methods/ModelExtension with the latest PHPStan master, results in a segmentation fault.

[nunomaduro]

Do you would like to continue this work of the upgrade?

[canvural]

Some parts, yes I'd like to fix. But for other parts solution is not easy. So I need help, or we need to discuss it.

[spaceemotion]

I am using the bare phpstan command in my Github Actions instead of the laravel-specific command (not sure why that is the case anymore). Didn't seem to make a difference, so dropping it got my personal vote (if it helps at all).

[nunomaduro]

Hummm, how about some default options? We need to make sure they are somehow ported.

[canvural]

The problem is, we are cloning the command definition here, and this has a different namespace because of PHAR. So later when we want to add options, it fails. So, we can just manually add every option that PHPStan defines. But don't know that's a good idea 😄

[canvural]

I fixed some more errors. There are still some small errors left, but the biggest one (one about the OptionsResolver class) still remains.

[canvural]

PHPStan 0.12 is gonna be released this week. (probably tomorrow)

Currently, we only have the OptionsResolver issue. Other than that, I think we are good to upgrade.

I'm writing the error here also so maybe other people can provide a solution too.

------ ------------------------------------------------------------------------- 
  Line   Console/OptionsResolver.php                                              
 ------ ------------------------------------------------------------------------- 
  75     Parameter #1 $option of method                                           
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputDefinition:  
         :addOption() expects                                                     
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputOption,      
         Symfony\Component\Console\Input\InputOption given.                       
  81     Parameter #1 $option of method                                           
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputDefinition:  
         :addOption() expects                                                     
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputOption,      
         Symfony\Component\Console\Input\InputOption given.                       
  87     Parameter #1 $option of method                                           
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputDefinition:  
         :addOption() expects                                                     
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputOption,      
         Symfony\Component\Console\Input\InputOption given.                       
  93     Method NunoMaduro\Larastan\Console\OptionsResolver::getDefinition()      
         should return Symfony\Component\Console\Input\InputDefinition but        
         returns                                                                  
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputDefinition.  
  93     Property NunoMaduro\Larastan\Console\OptionsResolver::$definition        
         (Symfony\Component\Console\Input\InputDefinition) does not accept        
         _HumbugBox74420952e3e6\Symfony\Component\Console\Input\InputDefinition.  
 ------ ------------------------------------------------------------------------- 

[szepeviktor]

Have I resolved it right?

[canvural]

Since there was some silence about this issue, I went ahead and removed the Laravel specific command we provide. In my opinion, this is a better solution. People will just use PHPStan like they normally would. And it's one less thing to maintain. In the past I see this command cause some issues.

Also, I introduced a new phpstan-baseline file. This was about a bug in PHPStan. But thanks to this new baseline future, we don't need to wait for it to be resolved.

And of course in the other branch (feat/adds-support-to-laravel-7) we should change the README to reflect this changes.

[szepeviktor]

People will just use PHPStan like they normally would.

+1

[szepeviktor]

Please use spaces in phpstan-baseline.neon

[mfn]

I went ahead and removed the Laravel specific command we provide

Thanks, that was a PITA especially for people who've already worked with phpstan before 👍

[nunomaduro]

Thanks for your work on this, can you double check why tests are passing?

I would like to re-discuss the php artisan code:analyse. I think it's important to keep this command, for new comers, and for default options.

Sorry for arriving late to the discussion.

[nunomaduro]

Made a tweet about this to collect some feedback: https://twitter.com/enunomaduro/status/1203956328714117121.

[ondrejmirtes]

Hi, MutatingScope is no longer marked as @internal, you were looking at some old dev-master code. The child class in Larastan was used for some hacks around array access I think. You should definitely test this on a real-world project to be sure. I'm fine with your Scope extending MutatingScope, that's why I made it overridable.

Currently, the biggest issue is in the OptionsResolver class.

PHPStan no longer allows using its dependencies directly in consumer code. The class should be rewritten - you can hardcode current PHPStan options and arguments, seems like the easiest approach.

[ondrejmirtes]

Anyway, please include me in this kind of discussion next time so you don't have to speculate how to solve something, it's valuable feedback for me too... Thanks.

[nunomaduro]

Funny results so far:

[ondrejmirtes]

It definitely shouldn't be dropped for technical reasons. The PHPStan CLI options are stable and do not change that often. They could be hardcoded instead of asking Symfony for them.

[canvural]

This is essentially a PHPStan extension. So users just install and use the PHPStan like they normally would. Like phpstan-mockery or phpstan-doctrine I think there is no value to introduce another abstraction layer on top of that. And in the past, we had many problems related to that command.

About MutatingScope, I tested it after removing our Scope implementation. (In two of my personal projects, also in work projects) And everything seems fine 👍

[canvural]

Travis build is failing because we are running PHPStan on a fresh Laravel instance. And that code has some issues.

We can either reduce the level we are running (currently at maximum) or we can try to find another way to test.

[nunomaduro]

I think there is no value to introduce another abstraction layer on top of that.

Let me share some of the values why we had this from the first place:

• Easy introduction of static analysis for Laravel developers: some Laravel developers don't even know what vendor/bin is, or what phpstan is. But they know what php artisan is, so it's just easy for them this way.
• It's more elegant calling php artisan code:analyse than ./vendor/bin/phpstan --options.
• Default parameters, like memory limit, folders, default configuration file, and others. ( maybe this can be different way now )
• Opens the door on having config/larastan.php under the config folder of an Laravel app. And this is the place where all configurations files of laravel packages lives. Those options would eventually be proxied down to the options on the phpstan cli.

[williamdes]

That might break something if people are using it in CI or some automated tools.

I agree, only if they are parsing the output.
Not changing the exit code and printing a line seems okay ?

[ondrejmirtes]

Outputting the notice to stderr should be fine.

[canvural]

I updated the PR.

But I'm not sure why Travis fails. And ideas?

[williamdes]

@canvural I think that I saw that phpstan removed orchestra dependencies
cc @ondrejmirtes

[ondrejmirtes]

PHPStan never depended on orchestra.

[ondrejmirtes]

It was removed from Larastan in August: 457af86

[canvural]

No. It's still there 😄

Edit: It's in the dev dependencies now. And Travis installs dev dependencies, so there shouldn't be a problem. 🤔

[canvural]

There is one error left in Travis build. It's about an error in the Laravel code itself. I sent a PR to fix it, but we are also testing on 5.8 and that fix will not be included there.

It's hacky but maybe we need another sed command like this one?

[ondrejmirtes]

PHPStan now supports stub files where you can override phpDocs from 3rd party code if they’re wrong or if you for example want to add generics.

[canvural]

I created a stub like this

<?php

namespace App\Http\Middleware;

use Illuminate\Auth\Middleware\Authenticate as Middleware;

class Authenticate extends Middleware
{
    /**
     * @param  \Illuminate\Http\Request  $request
     * @return string|null
     */
    protected function redirectTo($request);
}

and registered in extension.neon

But the error is still there. Also tried with @phpstan-return string|null in stub.

[ondrejmirtes]

The sed command you linked is doing something different. I don't know what are you trying to achieve. The stub you showed is overriding phpDoc for App\Http\Middleware\Authenticate::redirectTo() method. You can try to debug StubPhpDocProvider to see if it sees what you expect it to see.

Did you add the stub to stubFiles?

[canvural]

That sed command I linked was an example. It changes some doc block in the file to correct one. I was asking if we should do the same again.

Yes, I'm trying to change the App\Http\Middleware\Authenticate::redirectTo() return type with the stubs. But it seems it doesn't have any effect. It is registered in stubFiles. I'll try to debug.

[dshoreman]

That sed command I linked was an example. It changes some doc block in the file to correct one. I was asking if we should do the same again.

You could do something like sed -e 's/string/string|null/' -i app/Http/Middleware/Authenticate.php which would likely do the job just fine. Whether we should do that though is not my question to answer.

For the record, while I missed the Twitter poll I would also be in favour of removing the Artisan command—or at least making it optional. I use multiple testing & SA tools, none of which operate via artisan. Instead I use make phpstan for example, with all tools added as Makefile targets.

[ondrejmirtes]

The artisan command has always been optional. You can run PHPStan directly. If you use phpstan/extension-installer, you don’t even have to include Larastan’s extension.neon manually.

[dshoreman]

Sorry @ondrejmirtes, I should have clarified. While it's always been optional in the sense that you don't need to run it, when things do go wrong it can prevent any other artisan commands from being run (as was the case here before it was fixed).

If we can't remove it entirely, it'd be nice if there was a way to prevent it being auto-loaded to avoid these issues in future for those that use phpstan directly. Just my two cents though—command or not, this package has still saved a ton of time with all the custom rules, so thanks for everything you folks have done for Larastan!

[nunomaduro]

@ondrejmirtes makes any sense we require phpstan/extension-installer?

[ondrejmirtes]

@nunomaduro No it doesn't. This package is for "happy path" users only. It should always be installed only by the end user. It's possible that someone wouldn't want it - for example when they want to cherrypick enabled rules from other extensions or that they cannot use it for some other reason.

[canvural]

I guess this is ready to merge. @nunomaduro

I'll update the changelog in a seperate PR. Maybe after nunomaduro:feat/adds-support-to-laravel-7 is merged.

[nunomaduro]

I would like to review it and prepare some media content about it. What’s new, etc. can you give me one two extra days? @canvural

Ps: thousands of developers are grateful for your work on this.

[canvural]

This PR just upgrades PHPStan to 0.12, nothing new. But there are already some small changes in master

[nunomaduro]

Running a couple tests now on the branch nunomaduro:feat/adds-support-to-laravel-7.