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

Add generics types on ServiceEntityRepository using the LazyServiceEntityRepository #1611

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Add generics types on ServiceEntityRepository using the LazyServiceEntityRepository #1611

merged 1 commit into from
Jan 6, 2023

Conversation

jdecool
Copy link
Contributor

@jdecool jdecool commented Jan 6, 2023

#1599 has introduced a new ServiceEntityRepository definition using the LazyServiceEntityRepository.

This new definition doesn't declare the generic type that cause problems for static analysis when this service implementation is used.

@ostrolucky ostrolucky merged commit 251cd5a into doctrine:2.8.x Jan 6, 2023
@jdecool jdecool deleted the fix-ServiceEntityRepository-generics branch January 6, 2023 11:44
@stof stof added this to the 2.8.2 milestone Jan 6, 2023
@stof
Copy link
Member

stof commented Jan 6, 2023

@ostrolucky what do you think about releasing 2.8.2 with that fix, as it is a regression for anyone using SA tools ?

@ostrolucky
Copy link
Member

As it is I don't believe fix for SA alone warrants a release. 2.8.1 also isn't super important so users can hold off from updating to it. But let's see if this will get further feedback.

@stof
Copy link
Member

stof commented Jan 6, 2023

Well, the fix in #1599 actually fixes issues that happens depending on how you write the entity manager in your project. So it might actually be important depending on whether the project is impacted or no.

As it is I don't believe fix for SA alone warrants a release

For projects that require SA to pass in their CI (just like Doctrine does for instance), a regression in the types (the class not being generic anymore) can be a blocker.
As we have no other issues pending for 2.8.x, I don't see a benefit of delaying the release (getting duplicate reports for an already solved issue is a risk on the other hand)

@ro0NL
Copy link

ro0NL commented Jun 15, 2023

@jdecool, by any chance did you manage to make psalm recognize ServiceEntityRepository

all our repositories got wiped from the baseline :(

@jdecool
Copy link
Contributor Author

jdecool commented Jun 16, 2023

@ro0NL I don't use psalm (only phpstan).

This PR solve all issues with phpstan.

@ro0NL
Copy link

ro0NL commented Jun 16, 2023

@jdecool i figured :} cool 👍

@ro0NL
Copy link

ro0NL commented Jun 16, 2023

actually i think nobody uses psalm in symfony/doctrine projects 😂

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.

4 participants