[DoctrineBridge] Add an Entity Argument Resolver

[jderusse]

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	part of #40333
License	MIT
Doc PR	todo

This PR provides an Argument Resolver for Doctrine entities.

This would replace the SensioFramework's DoctrineParamConverter (in fact most of the code is copy/pasted from here) and helps users to disable the paramConverter and fix the related issue.

usage:

sensio_framework_extra:
    request:
        converters: false
services:
    Symfony\Bridge\Doctrine\ArgumentResolver\EntityValueResolver: ~

#[Route('/blog/{slug}')]
public function show(Post $post)
{
}

or with custom options

#[Route('/blog/{id}')]
public function show(
  #[MapEntity(entityManager: 'foo', expr: 'repository.findNotDeletedById(id)')]
  Post $post
)
{
}

[derrabus]

Can we add a constructor flag to make the resolver opt-in? I'd like to keep it dormant unless I use the attribute.

[jderusse]

Can we add a constructor flag to make the resolver opt-in? I'd like to keep it dormant unless I use the attribute.

Added a constructor option that make the resolver ignoring arguments without attribute

[derrabus]

Looks good so far. About the test: This whole feature is tested with a fully mocked entity manager. Since Doctrine ORM is an external library, this could make the tests brittle and hard to maintain if the ORM changes it's behavior (for whatever reason).

Shall we try to test agains a real EM with an in-memory SQLite DB?

[stof]

I think the main reason why you see the EM being lazy in most cases is because DoctrineBundle makes the EM lazy to make it resettable (by resetting the proxy), not to lazy-load its dependency graph.

[nicolas-grekas]

DoctrineBundle makes the EM lazy to make it resettable (by resetting the proxy), not to lazy-load its dependency graph.

Indeed, that answers my concerns!

The conflict between resolvers will be handled by registering the tag with the right priority (like we do for all other argumentResolvers).

I'd like to see this approach before adding an option to opt-in.

I rebased the PR on your fork and submitted jderusse#3 on top.
After that patch, I'll be 👍 as is.

[weaverryan]

Approve - other than fabbot.

In DoctrineBundle (where the wiring will live), the entire feature should be opt-in, right? I mean, it should require some sort of param_converter: true config in doctrine.yaml, which we could ship in the doctrine.yaml recipe. I think that's needed, otherwise I could imagine the auto-enabling of this feature could cause some edge-case weird situations.

[weaverryan]

minor - but getMapEntity() or getMapEntityAttribute() might be better - and $mapEntityAttribute instead of $options in the code. But this is purely for the enjoyment of me reading the code ;)

[chalasr]

Suggested change

	* Add `#[MapEntity]` with its corresponding `EntityArgumentResolver`
	* Add `#[MapEntity]` with its corresponding `EntityValueResolver`

[xabbuh]

fixed in abfdc54

[dunglas]

@weaverryan but when these edge cases appear, users can opt-out.

I prefer to have this kind of feature on by default, this helps making the framework easy to use for newcomers and to make the catchy features more discoverable. This also allow to not bloat the "default" config generated by Flex.

For instance, I would like to enable auto-validation by default. Sure there are some known edge cases, but they are easy to fix by disabling the feature for an entity or a property (require some googling but that's all). On the other hand, with the current situation (not enabled by default), only advanced users leverage this feature while it mostly targets newcomers who except (rightly imho) to not have to duplicate similar metadata (ORM, validation, PHPDoc...).

Bundles should be opiniated but configurable, enabling by default features with a high DX value like this one is a huge one for newcomers and for the simplicity of use of the framework.

[chalasr]

I’d at least make it explicitly opt-in when FrameworkExtraBundle is installed to prevent any conflicting situations or ambiguity

[derrabus]

I prefer to have this kind of feature on by default

I agree, the question is where this "on by default" happens. I'd prefer it to happen in the recipe which would make it consistent with other magic behavior like autowiring. And we would also make the "off switch" discoverable.

[dunglas]

The verbose config that we generate per default just to enable these features increase the perceived complexity of Symfony. Take a look at the default services.yaml file, it can be a bit scary and discouraging for newcomers just starting a new project.

IMHO the DX is better is it just works with as few generated config as possible by default, but you can tweak every features by reading the documentation.

[fabpot]

Thank you @jderusse.

[janklan]

I know this comment is coming rather late and possibly a bit off-topic, but I'm wondering if the converting behaviour I described in #47166 is a bug or a feature and may be worth addressing during the transition from SensioFrameworkExtraBundle to the new resolver?

[nicolas-grekas]

I opened doctrine/DoctrineBundle#1554 to wire this into doctrine-bundle.