Skip to content

Conversation

@jawira
Copy link
Contributor

@jawira jawira commented May 27, 2025

Fixes #423

SenseException
SenseException previously approved these changes Jun 1, 2025
@jawira
Copy link
Contributor Author

jawira commented Jun 13, 2025

Hi! is there anything I could do to help get this PR merged.
All checks have passed.
Thanks! :)

@greg0ire
Copy link
Member

Once EnumReflectionProperty::__contruct calls its parent constructor (as in PR #422) all methods like the following one can be safely removed (there's six of them):

Why don't you do it rightaway? Also, will there still be a point in keeping private readonly ReflectionProperty $originalReflectionProperty as a property if we do that?

I think this clashes a bit with the original design made in #248 by @IonBazan

@jawira
Copy link
Contributor Author

jawira commented Jun 14, 2025

Ah you are right, I spoke too quickly. I cannot remove constructor since method's signature is not the same as parent class.
However, I removed all "proxy methods" because they are not needed anymore :)

@jawira
Copy link
Contributor Author

jawira commented Jun 25, 2025

Hello I have implemented all requested changes, could you accept this MR please ?

@greg0ire
Copy link
Member

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

@jawira jawira force-pushed the reflection-constructor branch from a541771 to ac9f832 Compare June 25, 2025 21:12
SenseException
SenseException previously approved these changes Jun 25, 2025
@greg0ire
Copy link
Member

@IonBazan please review 🙏

@IonBazan
Copy link
Member

IonBazan commented Jul 2, 2025

Thanks @jawira - LGTM. However, my changes were actually based on @beberlei doctrine/orm#9304 so it's best to confirm once more with him.

Also good to add tests covering the changes you've made, reproducing the issue you reported.

@jawira jawira force-pushed the reflection-constructor branch from 44a3e03 to d27f24e Compare July 13, 2025 20:01
@jawira
Copy link
Contributor Author

jawira commented Jul 13, 2025

Hello @IonBazan, in doctrine/orm, ReflectionEnumProperty DO call the parent class.
This change was made in doctrine/orm#9381
The code has evolved since, so I have updated the current PR to match the following:

https://github.com/doctrine/orm/blob/2c01dac17367c203de6fab0331c82eca0d639cdc/src/Mapping/ReflectionEnumProperty.php#L14-L27

I have also added tests to cover errors I had.

@greg0ire greg0ire requested a review from beberlei July 13, 2025 20:26
@beberlei
Copy link
Member

I am not sure anymore why I did it this way originally, but if this works here then great. However be mindful that for Doctrine ORM 3.4+ above we changed the abstraction completly and this code here becomes obsolete.

@greg0ire greg0ire added this to the 4.0.1 milestone Jul 30, 2025
@greg0ire greg0ire merged commit f858271 into doctrine:4.0.x Jul 30, 2025
9 checks passed
@greg0ire
Copy link
Member

Thanks @jawira !

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.

EnumReflectionProperty throws Fatal Error

6 participants