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 #1002

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

Comments

@Harfusha
Copy link

Description

dereuromark/cakephp-ide-helper#353

\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

(Used annotations are changed by dereuromark/cakephp-ide-helper, but the issue is same)

Bake Version

PHP Version

8.3

@dereuromark dereuromark added this to the 3.x (CakePHP 5) milestone Aug 31, 2024
@LordSimal
Copy link
Contributor

In the meantime you can just use https://github.com/dereuromark/cakephp-ide-helper to automatically generate all the necessary annotations so static analysis is happy.

@dereuromark
Copy link
Member

Both Bake and IdeHelper are currently not adding <mixed> to iterable here.

@Harfusha Do u want to make a PR here?

@Harfusha
Copy link
Author

Harfusha commented Oct 7, 2024

And how should i implement it? Should i add or the specific entity? I would rather add specific entity.

@dereuromark
Copy link
Member

As specific as possible, sure.
We can do it here first, and then in parallel also port this to the IdeHelper.

@dereuromark
Copy link
Member

dereuromark commented Oct 15, 2024

Do all major IDEs understand/support this new "bleeding edge generated annotations"?
My latest PhpStorm 2024.2.3 Build #PS-242.23339.16, built on September 25, 2024
for example does not yet.

I wonder what issues this could bring for IDE usage (and developers).

For Psalm/PHPStan this update is sure useful.

Image

The red marked errors are Expected: variable

@Harfusha
Copy link
Author

Harfusha commented Oct 16, 2024

But this annotation was already in use.

* @method iterable<\Bake\Test\App\Model\Entity\UniqueField>|\Cake\Datasource\ResultSetInterface<\Bake\Test\App\Model\Entity\UniqueField> saveManyOrFail(iterable $entities, array $options = [])

Edit:
Or does phpstorm have problem only with this annotation in the functions params?

@dereuromark
Copy link
Member

Jep. Only input apparently.
You should see if there is an open Bug ticket for phpstorm etc we can plus one. Or if we have to create one.
But maybe we need to make this a Feature flag in IdeHelper then.

@Harfusha
Copy link
Author

Can you please test if we can use Entity[] instead of itterable?

@dereuromark
Copy link
Member

Yes, so

@method \TestApp\Model\Entity\BarBar[] patchEntities(\Cake\Datasource\EntityInterface[] $entities, array $data, array $options = [])

seems fine here on bake side, and rest could probably be on IdeHelper side.

@Harfusha
Copy link
Author

Ok i will update it this way :)

@Harfusha
Copy link
Author

Oh but itterable<> and Entity[] is not same, i dont know if this change is wise. I will revert it back to itterable

@dereuromark
Copy link
Member

dereuromark commented Oct 16, 2024

The main point is that any EntityInterface collection is accepted right? So EntityInterface[] kind of makes sense as an expression of a collection.
Just iterable is not saying too much I guess.

But sure, either way works.

@Harfusha
Copy link
Author

But when entities are fetched only using ->all() i will show errors, and you cannot set it to be array of entites, because that will mean that you cannot use any functions of the iterable like ->isEmpty(

@dereuromark
Copy link
Member

dereuromark commented Oct 16, 2024

It more and more sounds like we shouldnt touch the Bake templates and rather make it all an opt-in topic for IdeHelper.
We could revisit this in like a year if IDEs caught up by then.

@Harfusha
Copy link
Author

Yeah, only change i made in the end was array => mixed[] :D

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

No branches or pull requests

3 participants