Skip to content
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

fix: proxy with BackedEnum integer on identifier #997

Open
wants to merge 1 commit into
base: 3.4.x
Choose a base branch
from
Open

fix: proxy with BackedEnum integer on identifier #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/Proxy/ProxyGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
use function chmod;
use function class_exists;
use function dirname;
use function enum_exists;
use function explode;
use function file;
use function file_put_contents;
Expand Down Expand Up @@ -940,7 +941,12 @@ private function generateMethods(ClassMetadata $class)
if ($this->isShortIdentifierGetter($method, $class)) {
$identifier = lcfirst(substr($name, 3));
$fieldType = $class->getTypeOfField($identifier);
$cast = in_array($fieldType, ['integer', 'smallint'], true) ? '(int) ' : '';

if (ClassUtils::containsEnumType($method->getReturnType())) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@Gwemox Gwemox Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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();
        }

$cast = '';
} else {
$cast = in_array($fieldType, ['integer', 'smallint'], true) ? '(int) ' : '';
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I think that's the best way.

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@malarzm any news on this?

Copy link
Member

Choose a reason for hiding this comment

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

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 :(

Copy link
Author

Choose a reason for hiding this comment

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

How can I help ?

}

$methods .= ' if ($this->__isInitialized__ === false) {' . "\n";
$methods .= ' ';
Expand Down
34 changes: 34 additions & 0 deletions src/Util/ClassUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,21 @@

use Doctrine\Persistence\Proxy;
use ReflectionClass;
use ReflectionIntersectionType;
use ReflectionNamedType;
use ReflectionUnionType;

use function assert;
use function enum_exists;
use function get_class;
use function get_parent_class;
use function ltrim;
use function rtrim;
use function strrpos;
use function substr;

use const PHP_VERSION_ID;

/**
* Class and reflection related functionality for objects that
* might or not be proxy objects at the moment.
Expand Down Expand Up @@ -110,4 +117,31 @@ public static function generateProxyClassName($className, $proxyNamespace)
{
return rtrim($proxyNamespace, '\\') . '\\' . Proxy::MARKER . '\\' . ltrim($className, '\\');
}

/**
* Check if the type is an enum type or type containing an enum type
*
* @param ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null $type
*
*/
public static function containsEnumType($type): bool
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Union types are only allowed since PHP 8.0.

The PHP min version for this project is 7.1.

{
if (PHP_VERSION_ID <= 80100 || $type === null) {
return false;
}

if ($type instanceof ReflectionUnionType || $type instanceof ReflectionIntersectionType) {
foreach ($type->getTypes() as $unionedType) {
if (self::containsEnumType($unionedType)) {
return true;
}
}

return false;
}

assert($type instanceof ReflectionNamedType);

return enum_exists($type->getName());
}
}