fix: proxy with BackedEnum integer on identifier

[Gwemox]

If entity identifier return an int BackedEnum, Proxy Generator generate method with cast to int. But it's impossible to cast a BackedEnum to int.

Call getId() on proxy throw an error :
Warning: Object of class App\\Enum\\ProviderId could not be converted to int

Original entity:

    public function getId(): ProviderId
    {
        return $this->id;
    }

Generated proxy method before fix:

    /**
     * {@inheritDoc}
     */
    public function getId(): \App\Enum\ProviderId
    {
        if ($this->__isInitialized__ === false) {
            return (int)  parent::getId();
        }


        $this->__initializer__ && $this->__initializer__->__invoke($this, 'getId', []);

        return parent::getId();
    }

Generated proxy method after fix:

    /**
     * {@inheritDoc}
     */
    public function getId(): \App\Enum\ProviderId
    {
        if ($this->__isInitialized__ === false) {
            return  parent::getId();
        }


        $this->__initializer__ && $this->__initializer__->__invoke($this, 'getId', []);

        return parent::getId();
    }

[stof]

I would type the argument as ReturnType|null, to match the one of ReflectionMethod::getReturnType

[Gwemox]

Union types are only allowed since PHP 8.0.

The PHP min version for this project is 7.1.

[stof]

I would use self:: here. there is no reason to enable late static binding.

[Gwemox]

Yes, It's corrected

[malarzm]

Whichever way we go, this PR will need test to prevent regressions in the future :)

[malarzm]

I'm afraid this is only half-baked solution as without method's return type we still do not know what's inside. Maybe it'd be more efficient to generate an instanceof \BackedEnum check for the return statement?

[Gwemox]

Check instance in template, like this ?
from:

        if ($this->__isInitialized__ === false) {
            return (int)  parent::getId();
        }

to:

        if ($this->__isInitialized__ === false) {
            return  parent::getId() instanceof \BackedEnum ? parent::getId() : (int) parent::getId();
        }

[malarzm]

Or better yet: idea that needs somebody with a deeper ORM knowledge than me to be validated: what if we get rid of the cast instead? The proxied entity will have the field already casted to a correct type thanks to initial id hydration. This $cast construct here is 10 years old and removing it doesn't break any test so I think it's worth challenging.

[Gwemox]

Yes I think that's the best way.

If the method is typed, for the scalar types PHP already cast return value if needed?

[beberlei]

@malarzm AbstractProxyFactory::getProxy is responsible for setting the id field on the proxy, its not gone through hydration, instead it might just be the value passed to EntityManager::getReference($entity, $id) without casting. So getting rid of the cast would require to move the cast to a higher level in the ORM. Same for UnitOfWork::createEntity using getProxy, associated ids don't have types and are not converted to int for example.

A test in ORM would be:

$proxy = $em->getReference(CmsUser, "1");
$proxy->getId();

Unsure if we test this at the moment.

[malarzm]

Thanks @beberlei I'll try to have a closer look there in ORM 👍

[Gwemox]

@malarzm any news on this?

[malarzm]

Quoting myself from #998

Probably we won't end with this exact solution as it seems ORM has no tests for this behaviour either. The change looks OK ORM-wise, but I'm afraid it may hit users in an unexpected and hard to debug way.

Sadly I didn't have time to get back to this :(

[Gwemox]

How can I help ?