Skip to content

Change return type of getReflectionClass to ReflectionClass - #12519

Open
remco-pc wants to merge 1 commit into
doctrine:3.6.xfrom
remco-pc:patch-1
Open

Change return type of getReflectionClass to ReflectionClass#12519
remco-pc wants to merge 1 commit into
doctrine:3.6.xfrom
remco-pc:patch-1

Conversation

@remco-pc

Copy link
Copy Markdown

Updated the return type of getReflectionClass method to ensure it always returns a ReflectionClass instance. should fix: PHP Fatal error: Declaration of Doctrine\ORM\Mapping\ClassMetadata::getReflectionClass(): ?ReflectionClass must be compatible with Doctrine\Persistence\Mapping\ClassMetadata::getReflectionClass(): ReflectionClass in /Application/vendor/doctrine/orm/src/Mapping/GetReflectionClassImplementation.php on line 28

(both traits should have the same signature of a required ReflectionClass according to the latest version 4.2)

Updated the return type of getReflectionClass method to ensure it always returns a ReflectionClass instance.
should fix: PHP Fatal error:  Declaration of Doctrine\ORM\Mapping\ClassMetadata::getReflectionClass(): ?ReflectionClass must be compatible with Doctrine\Persistence\Mapping\ClassMetadata::getReflectionClass(): ReflectionClass in /Application/vendor/doctrine/orm/src/Mapping/GetReflectionClassImplementation.php on line 28
@@ -25,7 +25,7 @@ trait GetReflectionClassImplementation
*
* Can return null when using static reflection, in violation of the LSP

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you seen this comment?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@greg0ire yes, but why is isnt the interface defination then optional (nullable) i just did a hot fix) you guys should look in this, i am only a user who does not want too much french english (symfony) because i did not like it from the start , and a different configuration can trigger the other switch case of these threats

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure I fully understand your comment. Did you proofread it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@greg0ire why is in the if statement one not nullable, in the interface not nullable, but nulable at the else trait ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it has to do with different signatures for different major versions of the persistence library.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@greg0ire the bottom one is causing issues, and the other frameworks is probably not using this code, who made the comment, does it make sense to you, i did not read all the code, but when i remove the "null" return type there its working perfectly for me, the same as the top one, some malicious comments could be added, I might consider a rewrite script which automatically picks the top one for comparison, but all again extra work...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@remco-pc my bad, it's not about different major versions of persistence having different signatures.

Before #11533, it was like that. After #11533 it's conditionally not like that, and the condition depends on the version of persistence. I think I did this to maintain BC: people who upgrade the version of persistence are more likely to be OK with the BC break that is changing this signature. StaticReflectionService exists only on persistence 3. So upgrade to persistence 4 and you will not have the issue.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants