8000 Add type for AssociationMapping by VincentLanglet · Pull Request #9794 · doctrine/orm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add type for AssociationMapping #9794

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 1, 2022
Merged

Add type for AssociationMapping #9794

merged 3 commits into from
Jun 1, 2022

Conversation

VincentLanglet
Copy link
Contributor

No description provided.

@VincentLanglet
Copy link
Contributor Author

@greg0ire I tried to add all the key with some type to the AssociationMapping, by looking at the usage. I can't be sure if I have all the possible key, and if I didn't make any mistake about the optional ones ?, but I think it can be a good start.

This is useful when using

$this->em->getClassMetadata($associationMapping['targetEntity']);

because it avoid to write

/** @var class-string $targetEntity */
$targetEntity = $assoc['targetEntity'];
$targetClass = $this->em->getClassMetadata($targetEntity);

for phpstan.

I fixed two baseline errors, but I have a new one:

$associationMapping['type'] === 'manyToMany'

is throwing

Strict comparison using === between int and 'manyToMany' will always evaluate to false.

Can the type be a string ? I thought it would be the const public const MANY_TO_MANY = 8;.

@VincentLanglet VincentLanglet marked this pull request as ready for review May 29, 2022 11:21
@greg0ire
Copy link
Member

I'm on my phone so it's hard to help, but I'm OK with the "good start, possibly incorrect" way of doing things when it's just about phpdoc. Regarding the new issue, maybe try throwing when it's a string and then run the test suite, to see if that case is covered?

@VincentLanglet
Copy link
Contributor Author

Seems like it was just a bug since generateAssociationMappingPropertyDocBlock is considering $associationMapping['type'] to be an int.

The phpdoc added a lot of issues with psalm (most of them are possibly undefined offset); there are too much to solve so I updated the psalm baseline

@derrabus derrabus added this to the 2.13.0 milestone May 30, 2022
@derrabus
Copy link
Member

We should try to reduce the set of optional keys in that array. Otherwise we'll find ourselves adding lots of pointless assert() calls to reduce the baseline again. 😕

SenseException
SenseException reviewed May 30, 2022
View reviewed changes
@VincentLanglet
Copy link
Contributor Author

We should try to reduce the set of optional keys in that array. Otherwise we'll find ourselves adding lots of pointless assert() calls to reduce the baseline again. 😕

Sure, but do you see any key which are not optional and I made optional ?

I'll try to take a new look soon but I'm not sure that's possible since it depends on the type and the config of the relation.

@derrabus
Copy link
Member

For instance, most of the boolean keys look like it would make sense to always define them. Not sure if that'll help us though.

@VincentLanglet
Copy link
Contributor Author

For instance, most of the boolean keys look like it would make sense to always define them. Not sure if that'll help us though.

I thought you were talking about the phpdoc and the type I introduced.

Indeed it would make sens to aways define them. But I wouldn't risk those change in this PR because

  • I know nothing about how this code work so I'm not even sure how/where all the association mappings are generated.
  • This is much more risky to introduce a bug than my phpdoc update because there is sometimes checks like isset($mapping['key']) or $mapping['key) === null.
    I would recommend to do in another (or mutiple others) PR.

Do you think it must be done with this PR ?

derrabus
derrabus reviewed Jun 1, 2022
View reviewed changes
psalm-baseline.xml
Comment on lines +1095 to +1099
<file src="lib/Doctrine/ORM/ORMSetup.php">
<UndefinedClass occurrences="1">
<code>MemcachedAdapter::createConnection('memcached://127.0.0.1')</code>
</UndefinedClass>
</file>
Copy link
Member

Choose a reason for hiding this comment

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

This error looks odd and unrelated to your changes.

@derrabus
Copy link
Member
derrabus commented Jun 1, 2022

Do you think it must be done with this PR ?

No. This PR is a great step towards getting our types under control. Let's iterate from there.

@greg0ire greg0ire merged commit f84ecb2 into doctrine:2.13.x Jun 1, 2022
@greg0ire
Copy link
Member
greg0ire commented Jun 1, 2022

Thanks @VincentLanglet !

@VincentLanglet VincentLanglet deleted the associationMapping branch June 1, 2022 21:37
@greg0ire greg0ire mentioned this pull request Jun 9, 2022
Merged
@VincentLanglet VincentLanglet mentioned this pull request Jul 27, 2022
Merged
@VincentLanglet VincentLanglet mentioned this pull request Aug 6, 2022
Merged
@mpdude mpdude mentioned this pull request Jan 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SenseException SenseException SenseException approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Static Analysis
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.
4 participants
@VincentLanglet @greg0ire @derrabus @SenseException
0