-
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
Use array shapes where appropriate #10513
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The most Psalm errors occur in this file, because Psalm does not know that
joinTable
is always set for mappings that represent the owning side of a many to many relationship.As it stands, #10405 won't fix that either, because I did not create a separate type for owning sides, but maybe I should?
Before
After
For
ManyToOne
and forOneToMany
,isOwningSide
is redundant according to the docs. I think this could be modeled with an interface (not implementing it would mean the relationship is owned).Cc @mpdude
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.
How could/would Psalm know when/where you're dealing with an
AssociationMapping
that represents the owning side? Would have have to makeinstanceof
checks?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.
Yes, exactly! We would replace the guard check here, for instance:
orm/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php
Lines 37 to 46 in 31ff969
Looking at this, you can see how the owning and owned sides are very different in the case of the many to many relationship:
orm/lib/Doctrine/ORM/Mapping/ClassMetadata.php
Lines 1802 to 1886 in 0f77181
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.
Then let's see that we define the class hierarchy not only by technical criteria (i.e. which fields are common in several classes and extract those into a base class), but from a semantical point of view. What kind of checks do we have in the codebase?
For example
if ($mapping['isOwningSide']) ...
would becomeif ($mapping instanceof AssociationOwningSide) ...
or so.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 think we have two kind of checks: you just mentioned the first one, and the other one is against
$mapping['type']
, sometimes with the&
operator, for instance to detect if it's ato-many
, for instance here:orm/lib/Doctrine/ORM/PersistentCollection.php
Line 354 in 31ff969
We might need to introduce
ToOneAssociationMapping
andToManyAssociationMapping
interfaces. ATM I used those names for traits but I can change those toTo{One,Many}AssociationMappingTrait
I guess.The thing is, interfaces can't have properties, so it's only a viable option if we switch to methods instead.
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.
Ah but we only ever test for
TO_MANY
andTO_ONE
, never forONE_TO
orMANY_TO
(those constants don't even exist), so it should be possible to just include these as classes in the hierarchy. I will ditch the traits and do that instead.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 pushed a new commit on the other PR, switching from traits to classes.
Just realised that this might not be an issue because we should be able to change the phpdoc on
ClassMetadata::AssociationMapping
fromarray<string, AssociationMapping>
toarray<string, list-of-concrete-classes>
. Aninstanceof
check should allow SA to rule out some of the classes and to notice that some of them have common properties 🤞