[6.2] [doctrine-bridge] EntityValueResolver priority is too high and there is no way to be skipped by a custom ParamCoverter

[marforon]

Symfony version(s) affected

6.2

Description

EntityValueResolver introduced in Symfony 6.2 priority (110) is higher than RequestAttributeValueResolver priority (100) which makes ParamConverter unusable in some cases. Only UserValueResolver has a higher priority.

How to reproduce

Consider this controller action:

    #[Route('/asset-file/{assetFile}/distribute', name: 'distribute', methods: [Request::METHOD_POST])]
    #[ParamConverter('jwDistribution', converter: SerializerParamConverter::class)]
    public function distribute(AssetFile $assetFile, JwDistribution $jwDistribution): JsonResponse
    {
           return new JsonResponse();
    }

In this case, AssetFile and JwDistribution represent Doctrine's entities, but JwDistribution doesn't exist in repository and should be converted by a custom converter (serializer). Instead, EntityValueResolver is used also for JwDistribution and it obviously fails to convert (because it's entity object without identifier). That must be at least some kind of regression. Could we do something about it?

Possible Solution

At least
RequestAttributeValueResolver should has a higher priority than EntityValueResolver.

Additional Context

No response

[nicolas-grekas]

Thanks for the report. Can you please confirm that #48489 would fix the issue?

[marforon]

@nicolas-grekas thanks for the response. Unfortunately, it won't help. The problem is it will continue to execute a find method in EntityManager

symfony/src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php

Line 59 in 6780b02

} elseif (false === $object = $this->find($manager, $request, $options, $argument->getName())) {

and in my case, the identifier from API is null (new entity). It will throw this exception https://github.com/doctrine/orm/blob/2.13.x/lib/Doctrine/ORM/EntityManager.php#L452

It would help if the EntityValueResolver would be skipped if the identifier is null.

EDIT: getIdentifier() method returns JwDistribution object from the request, because ParamConverter already set the attribute in SerializerParamConverter, so this doesn't help

symfony/src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php

Line 104 in 6780b02

if (false === $id || null === $id) {

...

[nicolas-grekas]

OK thanks. Can you provide a small reproducing app that I could play with locally to work on this?

[stof]

having higher priority than RequestAttributeValueResolver is the intended purpose, as this supports essential features when using only the value resolver system.

But we should indeed add a condition to skip this find() when the attribute is already an object, to improve support for the case of mixing those value resolvers with the legacy ParamConverter system of SensioFrameworkExtraBundle.
Side note: you should convert your SerializerParamConverter to an argument resolver instead.

[marforon]

OK thanks. Can you provide a small reproducing app that I could play with locally to work on this?

I will do so later today or tomorrow.

having higher priority than RequestAttributeValueResolver is the intended purpose, as this supports essential features when using only the value resolver system.

But we should indeed add a condition to skip this find() when the attribute is already an object, to improve support for the case of mixing those value resolvers with the legacy ParamConverter system of SensioFrameworkExtraBundle. Side note: you should convert your SerializerParamConverter to an argument resolver instead.

Skipping the EntityValueResolver if the attribute is already an object would help. We are going to migrate to an argument resolver sooner or later, but to be honest I miss some option to skip this visitor pattern which iterates the whole stack of arguments resolvers (it's also not the best thing to do for performance reasons). It would be cool to add some attribute for parameters which would allow to define a specific argument resolver right away and skip the whole process with priority.

[nicolas-grekas]

But we should indeed add a condition to skip this find() when the attribute is already an object

I updated #48489 to follow this idea, does that work for you?

[marforon]

But we should indeed add a condition to skip this find() when the attribute is already an object

I updated #48489 to follow this idea, does that work for you?

Thank you! I tested it and it works for me.