-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Migrate collections to PHP 8 syntax #10084
Conversation
* {@inheritDoc} | ||
*/ | ||
public function matching(Criteria $criteria) | ||
public function matching(Criteria $criteria): ReadableCollection&Selectable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
437574b
to
1f8af96
Compare
*/ | ||
public function matching(Criteria $criteria) | ||
/** @return ReadableCollection<TKey, TValue>&Selectable<TKey, TValue> */ | ||
public function matching(Criteria $criteria): ReadableCollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add Selectable
to the native return type?
public function matching(Criteria $criteria): ReadableCollection | |
public function matching(Criteria $criteria): ReadableCollection&Selectable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't found how to do so, see #10084 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we're not adding the correct native return type because it'll make Psalm sad? Shall we rather ignore the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It haven't found how to ignore it: it's a crash, not a simple error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derrabus I've discussed this here: https://symfony-devs.slack.com/archives/C8SFXTD2M/p1665220038738889
I see 4 options:
- wait for Psalm 5 before merging this;
- merge without the intersection type, wait for a merge up between Psalm
4.x
andmaster
and switch all branches to Psalmmaster
branch, and add the type; - merge without the intersection type, wait for Psalm 5 to be released and add the type then
- switch to Psalm
master
branch now and add the type.
I don't see switching only 3.0.x to another version of Psalm as a reasonable option.
2 and 3 have my preference, what about you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted that into a new issue: #10118
* @var EntityManagerInterface|null | ||
*/ | ||
private $em; | ||
private array|null $association; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private array|null $association; | |
private array|null $association = null; |
1f8af96
to
dc2bd9e
Compare
Blocked by #10116 and a merge up |
No description provided.