orm icon indicating copy to clipboard operation
orm copied to clipboard

[Feature Request] Add source to ConversionException

Open radar3301 opened this issue 5 months ago • 0 comments

Feature Request

Q A
New Feature yes
RFC yes/no
BC Break no?

Summary

It would be helpful if the thrown ConversionException included the table and field of the source of the conversion exception:

Could not convert database value "7/18/25" to Doctrine Type datestr_immutable. Expected format: Y-m-d
#0 src\Doctrine\DBAL\Types\DateStrImmutableType.php(***): Doctrine\DBAL\Types\ConversionException::conversionFailedFormat()
#1 vendor\doctrine\orm\src\Internal\Hydration\AbstractHydrator.php(322): App\Doctrine\DBAL\Types\DateStrImmutableType->convertToPHPValue()
#2 vendor\doctrine\orm\src\Internal\Hydration\ObjectHydrator.php(323): Doctrine\ORM\Internal\Hydration\AbstractHydrator->gatherRowData()
#3 vendor\doctrine\orm\src\Internal\Hydration\ObjectHydrator.php(143): Doctrine\ORM\Internal\Hydration\ObjectHydrator->hydrateRowData()
#4 vendor\doctrine\orm\src\Internal\Hydration\AbstractHydrator.php(168): Doctrine\ORM\Internal\Hydration\ObjectHydrator->hydrateAllData()
#5 vendor\doctrine\orm\src\Persisters\Entity\BasicEntityPersister.php(912): Doctrine\ORM\Internal\Hydration\AbstractHydrator->hydrateAll()
#6 vendor\doctrine\orm\src\EntityRepository.php(110): Doctrine\ORM\Persisters\Entity\BasicEntityPersister->loadAll()
#7 vendor\doctrine\doctrine-bundle\src\Repository\ServiceEntityRepositoryProxy.php(72): Doctrine\ORM\EntityRepository->findBy()
#8 src\Controller\MyController.php(***): Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepositoryProxy->findBy()
#11 vendor\symfony\http-kernel\HttpKernel.php(183): App\Controller\MyController->getObject()
#12 vendor\symfony\http-kernel\HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#13 vendor\symfony\http-kernel\Kernel.php(182): Symfony\Component\HttpKernel\HttpKernel->handle()
#14 vendor\symfony\runtime\Runner\Symfony\HttpKernelRunner.php(35): Symfony\Component\HttpKernel\Kernel->handle()
#15 vendor\autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\HttpKernelRunner->run()
#16 public\index.php(12): require_once('...')
#17 {main}

This is a somewhat poor example, given that I'm using a custom type, and I should probably just fix my App\Doctrine\DBAL\Types\DateStrImmutableType->convertToPHPValue() to handle the "m/d/Y" format, but regardless of that minor nit, it turns out the offending field was 4 layers of relationships deep from the originally requested object.

It would be greatly beneficial, imo, to add some sort of improved messaging to the exception.

Proposed Fix

My quick initial naive implementation I did as a test to see if this was even actionable:

  1. wrap the entire switch statement in Doctrine\ORM\Internal\Hydration\AbstractHydrator->gatherRowData in a try/catch block:
catch (\Doctrine\DBAL\Types\ConversionException $ex) {
    throw new \Doctrine\DBAL\Types\ConversionException(\sprintf('%s, in field "%s.%s"', $ex->getMessage(), $dqlAlias, $fieldName), previous: $ex);
}
  1. add a catch block to AbstractHydrator->hydrateAll:
        catch (\Doctrine\DBAL\Types\ConversionException $ex) {
            throw new \Doctrine\DBAL\Types\ConversionException(
                str_replace(
                    array_map(fn($e) => "\"$e.", array_keys($resultSetMapping->aliasMap)),
                    array_map(fn($e) => "\"$e.", array_values($resultSetMapping->aliasMap)),
                    $ex->getMessage()
                ),
                previous: $ex->getPrevious()
            );
        }
  1. Resulting error message: Could not convert database value "7/18/25" to Doctrine Type datestr_immutable. Expected format: Y-m-d, in field "App\Doctrine\Entity\External\[TABLE].[FIELD]"

A more proper fix would be to either a. add className and fieldName properties to \Doctrine\DBAL\Types\ConversionException, or b. extend ConversionException and add those properties to the extended class (in some ORM exception namespace)

However, there are a few possible issues I can foresee:

  1. ObjectHydrator has a internal note: "Highly performance-sensitive code." I'm not sure what the impact of adding the try-catch exception handling would have on gatherRowData. It could (should?) probably be hoisted up to wrap the foreach block, as $fieldName would be in scope by the time the ConversionException happens.
  2. ~~$dqlAlias only exists in the default switch case. I don't know how to handle the other two cases. Maybe just the field name, and~~ the hydrateAll catch block tries to figure out the column owner, a la $rsm->declaringClasses[array_search($ex->field, $rsm->fieldMappings, true)]?

What are everyone's thoughts on this? I'd be willing to submit a PR if there's any interest, or rather lack of opposition, to adding this feature.

radar3301 avatar Sep 16 '24 19:09 radar3301