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

Remove override of template type for contains(): causing psalm errors #369

Conversation

annervisser
Copy link
Contributor

The override of this template type causes Psalm to throw an error whenever contains is called: https://psalm.dev/r/a2f9439bd2

Removing the extra type works as expected: https://psalm.dev/r/de5f58d52f

Fixes #368

Hikariii
Hikariii previously approved these changes May 8, 2023
@greg0ire
Copy link
Member

greg0ire commented May 8, 2023

I remember having to add this because either Psalm or PHPStan complained… maybe it is no longer required.

Should the same be done in AbstractLazyCollection as well?

Also, I notice this library still uses Psalm 4, we should upgrade 😓

@Hikariii
Copy link

Hikariii commented May 8, 2023

I have not checked different versions of psalm, we're using psalm 5. Maybe this isn't backwards compatible with 4. I don't know...

@greg0ire
Copy link
Member

greg0ire commented May 8, 2023

I don't think it is, otherwise the CI pipeline would fail, right?

@greg0ire
Copy link
Member

greg0ire commented May 8, 2023

I created a separate PR to deal with Psalm: #370

Please address my other comment and we can merge this.

@greg0ire
Copy link
Member

greg0ire commented May 9, 2023

Please also rebase, as I upgraded this lib to Psalm v5

@Hikariii
Copy link

Hikariii commented May 10, 2023

Should the same be done in AbstractLazyCollection as well?

Yes, contains() in AbstractLazyCollection has the same issue.

@greg0ire
Copy link
Member

I know, I was just asking politely that the issue be treated globally . No doubt it should be done.

The override of this template type causes Psalm to throw an error
whenever contains is called: https://psalm.dev/r/a2f9439bd2

Removing the extra type works as expected: https://psalm.dev/r/de5f58d52f

Fixes doctrine#368
@annervisser annervisser force-pushed the remove-override-of-template-type-causing-psalm-errors branch from aded07e to 71a25d7 Compare May 11, 2023 07:17
@annervisser
Copy link
Contributor Author

annervisser commented May 11, 2023

@greg0ire I was out of office for a couple of days, but I've now updated this PR

@greg0ire greg0ire merged commit 6026a70 into doctrine:2.1.x May 11, 2023
@greg0ire greg0ire added this to the 2.1.3 milestone May 11, 2023
@greg0ire
Copy link
Member

Thanks @annervisser !

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

Successfully merging this pull request may close these issues.

Psalm annotations ArrayCollection->contains() incorrect
4 participants