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

Extract read-only part of Collection #322

Merged
merged 2 commits into from Sep 1, 2022

Conversation

greg0ire
Copy link
Member

This allows us to mark the extracted interface as covariant, thus
allowing consumers to ensure through static analysis that a collection
will not be altered in a way that would no longer make it a collection
of items of a specific type.

Closes #298

lib/Doctrine/Common/Collections/ReadableCollection.php Outdated Show resolved Hide resolved
lib/Doctrine/Common/Collections/ReadableCollection.php Outdated Show resolved Hide resolved
docs/en/index.rst Outdated Show resolved Hide resolved
*
* @psalm-param TMaybeContained $element
*
* @psalm-return (TMaybeContained is T ? TKey|false : false)
Copy link
Member Author

@greg0ire greg0ire Aug 27, 2022

Choose a reason for hiding this comment

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

If I don't copy/paste this here, I get

ERROR: InvalidReturnType - lib/Doctrine/Common/Collections/ArrayCollection.php:261:21 - The declared return type '(TKey:Doctrine\Common\Collections\ReadableCollection as array-key)|false' for Doctrine\Common\Collections\ArrayCollection::indexOf is incorrect, got '(TKey:Doctrine\Common\Collections\ArrayCollection as array-key)|false' (see https://psalm.dev/011)
    public function indexOf($element)

from psalm, not sure why

Choose a reason for hiding this comment

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

I'd have to spend a bit more time looking to be sure, but I'm guessing that's probably vimeo/psalm#7553. Docblock inheritance and templates don't play well with one another in Psalm, probably due to the way TKey on ReadableCollection is a different type from TKey on ArrayCollection and the docblock inheritance isn't automatically translating it based on the @implements/@extends on the class/interface declaration.

@greg0ire greg0ire marked this pull request as ready for review August 27, 2022 15:03
@greg0ire greg0ire requested a review from stof August 27, 2022 15:03
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

There's still a failed build.

@greg0ire
Copy link
Member Author

greg0ire commented Sep 1, 2022

🤦 how did I not see that? Thanks for pointing it out!

@greg0ire
Copy link
Member Author

greg0ire commented Sep 1, 2022

Ah that would be because the build is 12 minutes old 🤔

@greg0ire
Copy link
Member Author

greg0ire commented Sep 1, 2022

Addressed in #327

@SenseException
Copy link
Member

Ah that would be because the build is 12 minutes old thinking

Yeah, someone tested the PR locally, then suddenly run into this problem and tested it in this PR. I don't know who that was though 😇

This allows us to mark the extracted interface as covariant, thus
allowing consumers to ensure through static analysis that a collection
will not be altered in a way that would no longer make it a collection
of items of a specific type.

Closes doctrine#298
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

I'm not sure about the impact on custom classes that extend a Doctrine Collection, but a sample worked fine locally.

@greg0ire greg0ire added this to the 1.8.0 milestone Sep 1, 2022
@greg0ire greg0ire merged commit 2b44dd4 into doctrine:1.8.x Sep 1, 2022
@greg0ire greg0ire deleted the readable-collection branch September 1, 2022 20:12
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.

None yet

4 participants