Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In PHPStan 1.12.0 with bleeding edge generated annotations @method for tables are reported #353

Closed
Harfusha opened this issue Aug 30, 2024 · 9 comments

Comments

@Harfusha
Copy link
Contributor

Harfusha commented Aug 30, 2024

\src\Model\Table\AccountsTable.php:55:Class App\Model\Table\AccountsTable has PHPDoc tag @method for method newEntity() parameter #1 $data with no value type specified in iterable type array.
\src\Model\Table\AccountsTable.php:55:Class App\Model\Table\AccountsTable has PHPDoc tag @method for method newEntity() parameter #2 $options with no value type specified in iterable type array.

This problem is with array type in params and with iterable type in params

@method \App\Model\Entity\Account newEntity(array $data, array $options = [])
This should be updated to this
@method \App\Model\Entity\Account newEntity(array<mixed> $data, array<mixed> $options = [])

@method \Cake\Datasource\ResultSetInterface<\App\Model\Entity\Account>|false saveMany(iterable $entities, $options = [])
This should be updated to this
@method \Cake\Datasource\ResultSetInterface<\App\Model\Entity\Account>|false saveMany(iterable<mixed> $entities, $options = [])
Or better to
@method \Cake\Datasource\ResultSetInterface<\App\Model\Entity\Account>|false saveMany(iterable<\App\Model\Entity\Account> $entities, $options = [])

PHPStan issue: phpstan/phpstan#11593

Edit more issues found:
@method \App\Model\Entity\Service[]|\Cake\Datasource\ResultSetInterface paginate($object = null, array $settings = [])
C:\Workplace\many\many\src\Controller\ServicesController.php:60:PHPDoc tag @method for method App\Controller\ServicesController::paginate() return type contains generic interface Cake\Datasource\ResultSetInterface but does not specify its types: T

@Harfusha
Copy link
Contributor Author

Not related to the errors, but maybe instead of interface in methods patchEntity, save, saveOrFail could be the specific entity?

@dereuromark
Copy link
Owner

dereuromark commented Aug 30, 2024

I agree that

 * @method \App\Model\Entity\Account|false save(\Cake\Datasource\EntityInterface $entity, array $options = [])

could indeed have the concrete Account entity typehhinted.

 * @method \App\Model\Entity\Account|false save(\App\Model\Entity\Account $entity, array $options = [])

But then PHPStan would complain in a few more cases where the current annotations and methods "only" return the
typehinted interface.
So I am not sure if we want to do this.
It could be an option flag maybe? useConcreteEntities true/false ?

As for the iterable vs ``iterable<\App\Model\Entity\Account>` - I think you should first do a PR to the bake repo and if accepted there, we can adjust it here easily as follow up.

I didn't know that you can use this syntax in the annotations for method arguments.

@Harfusha
Copy link
Contributor Author

I didnt knew that either :D I will make issue in bake repo for this

useConcreteEntities flag seems reasonable

@dereuromark
Copy link
Owner

@Harfusha

useConcreteEntities flag seems reasonable

But as I said: This means that any result from other methods could be considered "wrong" by static analyzers due to the strict concrete requirement then. I am still not too sure it is a good idea. But I can see why some would find it useful.

@dereuromark
Copy link
Owner

I just released
But sth is wrong with the annotations

* @method \Cake\Datasource\ResultSetInterface<\Community\Model\Entity\Article><\Community\Model\Entity\Article> paginate($object = null, array $settings = [])

gets generated now.

@dereuromark
Copy link
Owner

2.5.1...2.5.2 - fixed

@dereuromark
Copy link
Owner

Are you sure the array|string => string was OK for get()?
I now get

  Line   Controller/Admin/EventLinksController.php                                              
 ------ --------------------------------------------------------------------------------------- 
  36     Parameter #2 $finder of method App\Model\Table\EventLinksTable::get() expects string,  
         array<string, array> given.                                                            
  69     Parameter #2 $finder of method App\Model\Table\EventLinksTable::get() expects string,  
         array<string, array> given.    

All over the place.

@Harfusha
Copy link
Contributor Author

Harfusha commented Oct 17, 2024

But finder as array is depricated
image

This was changed based on this:
cakephp/bake#1010 (comment)

@dereuromark
Copy link
Owner

dereuromark commented Oct 17, 2024

Yeah, but variadic usage seems to still need it, too.

    $eventImport = $this->EventImports->get($id, ...[
        'contain' => ['Events'],
    ]);

So I guess we need to keep it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants