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.

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

[Feature Request] Add source to ConversionException #11600

Open
radar3301 opened this issue Sep 16, 2024 · 0 comments
Open

[Feature Request] Add source to ConversionException #11600

radar3301 opened this issue Sep 16, 2024 · 0 comments

Comments

@radar3301
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant