Object that are not entities are checked for Metadata without cache

[bastnic]

This is a followup of doctrine/dbal#3838, I just want to check if maintainers see a room for improvement.

TL;DR: when we check metadata for doctrine entities, there is a cache that prevents checking for metadata across requests and there is memoization to prevent hitting on the cache. When passing objects that are not doctrine entities with fallback there is no cache but memoization, and without fallback there is none of that.

There are at least two paths that trigger that behavior (just to show that this is not an easy fix in userland):

1/ doctrine query with datetime parameter without the third parameter "datetime" (dev and prod)

$q = $em->createQuery('SELECT d from App\Entity\DataSample d WHERE d.referenceDate = :date');
$q->setParameter('date', Carbon::now(), 'datetime');

• https://github.com/doctrine/orm/blob/e77091399a4475630a833fc33bff0d54c194511a/lib/Doctrine/ORM/AbstractQuery.php#L402
• https://github.com/doctrine/orm/blob/e77091399a4475630a833fc33bff0d54c194511a/lib/Doctrine/ORM/UnitOfWork.php#L2491
• https://github.com/doctrine/orm/blob/e77091399a4475630a833fc33bff0d54c194511a/lib/Doctrine/ORM/EntityManager.php#L286

• persistence/lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

  Line 161 in fdaaa50

  public function getMetadataFor(string $className)

• persistence/lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

  Line 206 in fdaaa50

  throw $loadingException;

This is fixable in userland, but quite not fun.

This is a blackfire profile in prod env with 20 sql requests with a datetime parameter WITHOUT the third parameter set. The after is a simple test at the beginning of the getMetadataFor to avoid loading the metadata stack:

        if ($className === 'DateTime') {
            throw new MappingException();
        }

2/ symfony validation (dev only)

• https://github.com/symfony/symfony/blob/4c1ca329a440fbf98ac90208a79292606d4cd92c/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php#L101
• https://github.com/symfony/symfony/blob/4c1ca329a440fbf98ac90208a79292606d4cd92c/src/Symfony/Component/Validator/Mapping/Loader/LoaderChain.php#L54
• https://github.com/symfony/symfony/blob/4c1ca329a440fbf98ac90208a79292606d4cd92c/src/Symfony/Bridge/Doctrine/Validator/DoctrineLoader.php#L51
• https://github.com/doctrine/orm/blob/e77091399a4475630a833fc33bff0d54c194511a/lib/Doctrine/ORM/EntityManager.php#L286

• persistence/lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

  Line 161 in fdaaa50

  public function getMetadataFor(string $className)

• persistence/lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

  Line 206 in fdaaa50

  throw $loadingException;

3/ doctrine embedable classes

---

I don't know the best fix for that but I figures that on the life on a request / cache, this should not happen. Even I who know this bug, I forgot to add the third parameter, it should really be resolved in doctrine/persistence.

We could:

• cache the exception and add some instanceof / throw somewhere.
• cache null and return an exception if null, we should change the isset by array_key_exists

[greg0ire]

IMO the most BC way would be to add a new array, and cache exceptions in that array, that way you are sure to throw the same type of exception as before. The issue is that it will be an exception with an outdated stack trace. It's an issue but ideally (in the future), that code path should not even be used by apps. There should be a new method (that would have been named hasMetadataFor, but will not because that name is already taken for something that seems completely useless) that returns a boolean and that you would be supposed to check before calling loadMetadataFor, because relying on exceptions for things that are not exceptional does not make a lot of sense. Since loadedMetadata is exposed through getLoadedMetadata, touching that array is a no-go, even though it is private and even though that method does not seem to relied on much.