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

Introduce Voter stub #326

Merged
merged 2 commits into from
Jan 31, 2023
Merged

Introduce Voter stub #326

merged 2 commits into from
Jan 31, 2023

Conversation

VincentLanglet
Copy link
Contributor

Does such a stub require a test ?

@ondrejmirtes
Copy link
Member

I'm not sure that everyone who extends Voter wants this functionality. In case they do, try introducing these generics into Symfony directly.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Jan 19, 2023

I'm not sure that everyone who extends Voter wants this functionality.

Same could be said for almost every stub I would say.
For instance I get lot of trouble with this stub #322

I don't consider this stub as an hack or a wrong stub.
And I thought this was one of the purpose of skipCheckGenericClasses, people can skip this stub.
Or just Voter<string, mixed> will do nothing.

In case they do, try introducing these generics into Symfony directly.

Symfony is a kinda slow adopter about new annotations as shown in symfony/symfony#46613
Also, they only accepts such things on new branch, which means it would be only available on Sf 6.3.

But I can start by this to get feedback and decide after for phpstan-symfony, sure. 👍
Edit: opened symfony/symfony#49033

@VincentLanglet
Copy link
Contributor Author

I'm not sure that everyone who extends Voter wants this functionality. In case they do, try introducing these generics into Symfony directly.

Hi @ondrejmirtes, the template was accepted by symfony in 6.3 symfony/symfony#49033
Do we merge this to add the template in older symfony version ?

*
* @param mixed $subject
*
* @phpstan-assert-if-true TSubject $subject
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, fixed.

@ondrejmirtes ondrejmirtes merged commit 44aef78 into phpstan:1.2.x Jan 31, 2023
@ondrejmirtes
Copy link
Member

Thank you.

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

Successfully merging this pull request may close these issues.

2 participants