search

CakeDC / cakephp-phpstan

CakePHP extension for PHPStan
Other
33 stars 6 forks source link

PHPStan issues with get() and others #20

Closed dereuromark closed 8 months ago

dereuromark commented 11 months ago

I added the following rules to more clearly see where PHPStan doesnt see the proper object type:

rules:
    - Symplify\PHPStanRules\Rules\Explicit\NoMixedPropertyFetcherRule
    - Symplify\PHPStanRules\Rules\Explicit\NoMixedMethodCallerRule

Turns out almost none of the annotations actually work out

  83     Anonymous variables in a "$articleSet->..." property fetch can lead to false dead property. Make sure the variable type is known 
$articleSet = $this->Articlesets->get($data['id']);
$data['price'] = $articleSet->price; // Doesnt recognize it, PHPStorm does (ArticleSet|EntityInterface)

Also the relations annotations dont seem to work yet:

  * @property \App\Model\Table\ArticlesTable&\Cake\ORM\Association\HasMany $Articles

Makes it not find it anymore

$this->Orders->Articles->find()->where(['order_id' => $id])->all();

Fails with

 Anonymous variable in a `$this->Orders->Articles->find()->where(['order_id' => $id])->...()` method call can lead to false dead methods. Make sure the variable type is known

It only works if we remove the relation part and only keep the table name:

  * @property \App\Model\Table\ArticlesTable $Articles

Should we then annotate it without the relation part?

rochamarcelo commented 11 months ago

About the $data['price'] = $articleSet->price; // Doesnt recognize it, PHPStorm does (ArticleSet|EntityInterface) I think phpstan should complain since EntityInterface does not have price property only ArticleSet

About the relations annotation, I expect to not have issue with Intersection types, the doc says it is supported https://phpstan.org/blog/union-types-vs-intersection-types .

In a perfect world would be nice to have a phpdoc saying it is a HasMany object containing ArticlesTable object, we have to think of a good solution.

dereuromark commented 11 months ago

We know that get() on that table with that entity should return the concrete object We should make PHPStan believe that too, it should not error as the pure amount of needed inline annotations is already crazy and we shouldnt be forced to add more where the results are obvious IMO As such, we should make sure that get() only returns the concrete object as already somewhat specified in annotation.

rochamarcelo commented 11 months ago

I mean if you have an annotation saying it may return ArticleSet|EntityInterface phpstan won't know that EntityInterface.price exists.

We could ignore the annotation and add a custom logic to detect the type as we do to detect if a custom table class exists, keep in mind that we probably can't cover the cases when someone use a non-convetional class name for entity.

Addittionally I was researching about generics annotation with phpstan, maybe it can improve the CakePHP core annotations with generics for entity class, and table class for associations, then we could have these in ours models.

@template Entity of \App\Model\Entity\Article / @property HasMany<\App\Model\Table\ArticlesTable> $Articles

Anyway, these are just some ideas, I don't have proof that adding generics annotation will work.

dereuromark commented 11 months ago

Custom logic sounds fair enough for now

dereuromark commented 11 months ago

I prepared https://github.com/dereuromark/cakephp-ide-helper/pull/335

What implications does this have on existing code and PHPStans ability to understand it?

rochamarcelo commented 11 months ago

This should fix issues related to Symplify\PHPStanRules\Rules\Explicit\NoMixedPropertyFetcherRule and cover cases:

$user = $this->Notes->Users->get($note->user_id);
$user->role = 'user';

$this->Notes->User->myCustomFunction();

Now you mentioned cakephp-ide-helper I noticed that PHPStorm autocomplete is partially working, it works fine for custom methods inside your table but fail to understand the custom @method annotations and with entity properties. This is odd because some time it shows the correct method and sometime show multiple method.

Here it shows the custom @method save defined in AppUsersTable but PHPStorm fails to understand $user->role:

Screenshot from 2023-11-27 22-11-03

Tested on PhpStorm 2023.2.4 I believe I started to get errors after Invalidated cache. Could you check if PHPStorm autocomplete works fine for you @dereuromark ?

dereuromark commented 11 months ago

Yes, the new syntax would break IDE autocomplete, so it seems like rather a too strong issue compared to the small benefit of less inline annotations for PHPStorm. I wonder if PHPStorm is already aware of it.

rochamarcelo commented 11 months ago

Yes. Trait supports was add on the latest version 2023.2 (It is not available on 2023.1) It seems the feature is still in development. I've just an issue report

https://youtrack.jetbrains.com/issue/WI-75239/Autocomplete-and-go-to-declaration-does-not-work-properly-with-mixin-and-generics?clearDraft=true&description=%0A%0APS-232.10227.13,%20JRE%2017.0.9%2B7-b1000.46x64%20JetBrains%20s.r.o.,%20OS%20Linux(amd64)%20v5.4.0-166-generic,%20screens%202560.0x1080.0

dereuromark commented 11 months ago

I am on 2023.2.3

dereuromark commented 11 months ago

I can confirm that the phpstan run is now OK, once I used my branch of IdeHelper to auto-change all to new syntax. But then I lose all IDE compatibility, so its a no go for now.

We should open a ticket with PHPStorm to fast track this there.

rochamarcelo commented 11 months ago

@dereuromark have this one https://youtrack.jetbrains.com/issue/WI-75239/Autocomplete-and-go-to-declaration-does-not-work-properly-with-mixin-and-generics ~5 days ago and no updated from them , I think they prioritize the ones with high votes, so please vote

dereuromark commented 11 months ago

Upvoted

I merged https://github.com/dereuromark/cakephp-ide-helper/commit/33d8f43327f9666cf2e4acb53ef79bb7d810779d in, but with a feature flag to manually enable, as this is probably not usable as is for now.

dereuromark commented 11 months ago

PHPStorm 2023.3 was just released a few days ago. We should check if things got better.

PS: There is also https://youtrack.jetbrains.com/issue/WI-63261/Generics-support-by-passing-template-TValue-into-template-implements-IteratorAggregateint-TValue which seems a bit related to.

rochamarcelo commented 8 months ago

At https://github.com/CakeDC/cakephp-phpstan/blob/feature/cakephp-rules-001 I've added an 'extension' to fix the bad intersection phpDoc to the new one (generic Association

.

PS: The change works with the rule NoMixedPropertyFetcherRule

@dereuromark Could you try this branch https://github.com/CakeDC/cakephp-phpstan/tree/feature/cakephp-rules-001 ?

dereuromark commented 8 months ago

Quite a few anonymous variables go indeed away, e.g. 20+ of

  257    Anonymous variable in a `$this->Images->find()->where(['id' =>
         $id])->...()` method call can lead to false dead methods. Make sure
         the variable type is known
  265    Anonymous variable in a `$this->Images->find()->where(['article_id'
         => $deletedImage->article_id, 'type' => $deletedImage->type])->...()`
         method call can lead to false dead methods. Make sure the variable
         type is known

are gone with this branch

but other issues seem to appear, e.g.

  141    Call to App\Model\Table\ArticlesetsTable::belongsToMany with unknown
         option "joinTable".
  163    Call to App\Model\Table\ArticlesetsTable::hasMany could not find the
         class for "Images"

for e.g.

        $this->belongsToMany('Styles', [
            'foreignKey' => 'articleset_id',
            'targetForeignKey' => 'style_id',
            'joinTable' => 'articlesets_styles',
        ]);

which is definitly valid option/key.

Same issue for

  54     Call to App\Model\Table\WarehousesTable::belongsTo with unknown
         option "sort".

Otherwise it looks good :)

rochamarcelo commented 8 months ago

Thanks @dereuromark, I still have to add tests to cover belongsToMany.

As reference that branch adds a few rules, one of them is to check the association options type.

For `Call to App\Model\Table\WarehousesTable::belongsTo with unknown option "sort" this error seems correct, checking the doc and source code the sort key is only used for HasMany and BelongsToMany Association

And about Call to App\Model\Table\ArticlesetsTable::hasMany could not find the class for "Images" do you have ImagesTable ?

dereuromark commented 8 months ago

Good points!

dereuromark commented 8 months ago

https://github.com/CakeDC/cakephp-phpstan/pull/28#issuecomment-1960694570

rochamarcelo commented 8 months ago

@dereuromark Thank you, I'll take a look.

dereuromark commented 8 months ago

Anything left or shall we close this?

dereuromark commented 8 months ago

I do get an error with the latest update, however

     Error                                                                            
 -- --------------------------------------------------------------------------------- 
     Internal error: Internal error: Method findupcoming() was not found in           
     reflection of class Cake\ORM\Table. while analysing file                         
     /shared/httpd/de/src/Controller/DancesController.php                             
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method finduser() was not found in reflection    
     of class Cake\ORM\Table. while analysing file                                    
     /shared/httpd/de/src/Controller/EventPlacesController.php                        
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findupcoming() was not found in           
     reflection of class Cake\ORM\Table. while analysing file                         
     /shared/httpd/de/src/Controller/CountriesController.php                          
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findupcoming() was not found in           
     reflection of class Cake\ORM\Table. while analysing file                         
     /shared/httpd/de/src/Controller/LocationController.php                           
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findactive() was not found in reflection  
     of class Cake\ORM\Table. while analysing file                                    
     /shared/httpd/de/src/Controller/UserCardsController.php                          
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method finduser() was not found in reflection    
     of class Cake\ORM\Table. while analysing file                                    
     /shared/httpd/de/src/Controller/AccountController.php                            
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findactive() was not found in reflection  
     of class Cake\ORM\Table. while analysing file                                    
     /shared/httpd/de/src/Controller/CitiesController.php                             
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findupcoming() was not found in           
     reflection of class Cake\ORM\Table. while analysing file                         
     /shared/httpd/de/src/Controller/PlacesController.php                             
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method finduser() was not found in reflection    
     of class Cake\ORM\Table. while analysing file                                    
     /shared/httpd/de/src/Controller/EventProfilesController.php                      
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findupcoming() was not found in           
     reflection of class Cake\ORM\Table. while analysing file                         
     /shared/httpd/de/src/Controller/EventsController.php                             
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findsearch() was not found in reflection  
     of class App\Model\Table\EventParticipationsTable. while analysing file          
     /shared/httpd/de/src/Controller/Admin/EventParticipationsController.php          
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Internal error: Internal error: Method findsearch() was not found in reflection  
     of class Cake\ORM\Table. while analysing file                                    
     /shared/httpd/de/src/Controller/CalendarController.php                           
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Child process error (exit code 1):                                               
     Internal error: Internal error: Method finddance() was not found in reflection   
     of class Cake\ORM\Table. while analysing file                                    
     /shared/httpd/de/src/Model/Filter/EventsApiFilterCollection.php                  
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Child process error (exit code 1):                                               
     Internal error: Internal error: Method finddistance() was not found in           
     reflection of class Data\Model\Table\StatesTable. while analysing file           
     /shared/httpd/de/src/Command/UpdateCityStatesCommand.php                         
     Run PHPStan with -v option and post the stack trace to:                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml           
     Child process error (exit code 1):                                               
     Child process error (exit code 1):
dereuromark commented 8 months ago

The issue comes with 3.1.0, I just checked.

rochamarcelo commented 8 months ago

@dereuromark Could you run phpstan with --debug option and post the result here? if you could share a bit of code of where the finder is defined would be great.

rochamarcelo commented 8 months ago

If you can please run phpstan using the branch https://github.com/CakeDC/cakephp-phpstan/tree/issue/fix-phpstan-has-method-check

dereuromark commented 8 months ago

Seems to work but I got another issue still it seems

289 Call to App\Model\Table\EventsTable::find is missing required finder option "options". 

$options = [
            'year' => $this->Calendar->year(),
            'month' => $this->Calendar->month(),
        ];

        $events = $this->Events->find('calendar', $options); // line 289

the behavior code

    public function findCalendar(SelectQuery $query, array $options): SelectQuery {

The old syntax should probably not error

rochamarcelo commented 8 months ago

Right, we should support the old syntax

dereuromark commented 8 months ago

See also https://github.com/dereuromark/cakephp-sandbox/pull/73 (PHPStan result) for a few more issues that appeared with 3.1.0

=> https://github.com/dereuromark/cakephp-sandbox/actions/runs/8043007791/job/21964450222?pr=73#step:10:12

rochamarcelo commented 8 months ago

Issues fixed on https://github.com/CakeDC/cakephp-phpstan/tree/issue/fix-phpstan-has-method-check . Should be released soon.

dereuromark commented 8 months ago

I can confirm that almost all issues are gone But

    // Instead of primary key `id` and ->get($id) we work on `uuid` field now for public access
    $field = $this->ExposedUsers->getExposedKey();
    $exposedUser = $this->ExposedUsers->find('exposed', [$field => $uuid])->firstOrFail();

with

    public function findExposed(SelectQuery $query, string $uuid): SelectQuery {

seems to be left

115 Call to Sandbox\Model\Table\ExposedUsersTable::find is missing required finder option "uuid".

Not sure if that is my issue or not. Afaik the "old way" should still delegate into the new named way.

I also tried 

$exposedUser = $this->ExposedUsers->find('exposed', ...[$field => $uuid])->firstOrFail();

which is supposed to use named then, but the same error still shows.

My guess is that it is the dynamic $field part.

My recommendation: Skip the validation if there is such a dynamic ...[] or [$... => ...] part present?

rochamarcelo commented 8 months ago

This was fixed, composer (packagist) picked an older version

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.