Bugfix: Parent methods in extended EnumReflectionProperty didn't work.

[luzrain]

EnumReflectionProperty extends ReflectionProperty without ReflectionProperty::__constructor() call making it impossible to use parent methods such as ReflectionProperty::getAttributes() and others.

[derrabus]

Not calling the parent constructor was on purpose because this class is meant to be a decorator. Calls to methods like the ones you mentioned should be proxied to $this->originalReflectionProperty instead. We apparently had no need for them yet, but I'd happily merge any PR that adds those two methods to the EnumReflectionProperty class.

[luzrain]

If it is a decorator, then it should implement all parent methods, not just the two I mentioned. Without them, EnumReflectionProperty is inconsistent, and calling any of the parent methods will throw an exception, affecting third-party software that relies on the ReflectionProperty interface (this is what I encountered with).

I've seen the same solution in Doctrine ORM ReflectionEnumProperty.php and it was accepted.
I think it would be a completely sufficient solution in this case.

[derrabus]

If it is a decorator, then it should implement all parent methods, not just the two I mentioned.

Yes, it's currently incomplete.

Without them, EnumReflectionProperty is inconsistent,

Your change makes it an inconsistent decorator.

affecting third-party software that relies on the ReflectionProperty interface (this is what I encountered with).

Can you elaborate on this? I'm having a hard time finding out where this class is actually used.

I've seen the same solution in Doctrine ORM ReflectionEnumProperty.php and it was accepted. I think it would be a completely sufficient solution in this case.

I think, this change was okay-ish for the ORM because it's an internal class there. I would evaluate it differently for this library.

---

The existence of a EnumReflectionProperty in the ORM codebase makes me question even more why Persistence ships this class at all. We should either make the ORM leverage EnumReflectionProperty from Persistence or kill the Persistence version. WDYT @greg0ire?

[luzrain]

Can you elaborate on this? I'm having a hard time finding out where this class is actually used.

I encountered with issues trying to dump entity fetched by MongoDB ODM in symfony dumper.
I'm not sure about how it is constructed internally but the error I faced with came from here.
ReflectionCaster::castProperty() method relies on ReflectionProperty which is not properly decorated by EnumReflectionProperty and throws an exception when symfony dumper tryin to get access to ReflectionProperty methods.

I think, this change was okay-ish for the ORM because it's an internal class there. I would evaluate it differently for this library.

Isn't it internal as well?

[derrabus]

Can you elaborate on this? I'm having a hard time finding out where this class is actually used.

I encountered with issues trying to dump entity fetched by MongoDB ODM in symfony dumper.

Okay, so the ODM does use it. That answers my question.

the error I faced with came from here. ReflectionCaster::castProperty() method relies on ReflectionProperty which is not properly decorated by EnumReflectionProperty and throws an exception when symfony dumper tryin to get access to ReflectionProperty methods.

Right, we should fix the decorator then.

I think, this change was okay-ish for the ORM because it's an internal class there. I would evaluate it differently for this library.

Isn't it internal as well?

It's not internal to the persistence library, no.

[greg0ire]

The existence of a EnumReflectionProperty in the ORM codebase

git log -S EnumReflectionProperty yields nothing when run from the ORM repository, so… not sure what you are talking about 😅

EDIT: Ah, it's called one way in the ORM, and the other way in persistence 😅

The ORM class was added in doctrine/orm#9304
The persistence class was added in #248, the stated goal is to reduce duplication, so… yeah 🤣

[luzrain]

So what is the conclusion? How do you see the solution?
Redefining all of the methods of ReflectionProperty does not seem to be a good option either, because the list of available methods varies in different php versions (7.4, 8.0, 8.1) ,and since this package supports php >= 7.2, we should deal with it somehow.

[derrabus]

So what is the conclusion? How do you see the solution?

Let's add the missing proxy methods that we need. Alternatively, we could consider patching VarDumper to not treat this EnumReflectionProperty like an actual ReflectionProperty from PHP core.

[mustanggb]

I've also encounted this with the same use-case as: #342 (comment)
(Symfony VarDumper of MongoDB ODM object with an Enum field)

Spits out the error: Internal error: Failed to retrieve the reflection object

Confirming this merge request does "fix" it, meaning gets past the error, not sure if it actually does what's intended or not though.

I also tried adding the getDocComment and getModifiers methods, which I think is what #342 (comment) was suggesting, and that also works.

    /**
     * {@inheritDoc}
     */
    public function getDocComment(): string|false
    {
        return $this->originalReflectionProperty->getDocComment();
    }

    /**
     * {@inheritDoc}
     */
    public function getModifiers(): int
    {
        return $this->originalReflectionProperty->getModifiers();
    }

[malarzm]

I also tried adding the getDocComment and getModifiers methods, which I think is what #342 (comment) was suggesting, and that also works.

#351 was merged and released last week. So given it fixes same issue we're good to close this one?

[mustanggb]

#351 was merged and released last week. So given it fixes same issue we're good to close this one?

Ah perfect, yes that should resolve this, good to close I'd say.

[malarzm]

Cool! Thanks @luzrain for your work on this PR!