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

Leverage targeted resolvers #773

Closed
wants to merge 4 commits into from
Closed

Conversation

franmomu
Copy link
Contributor

Closes #770

Symfony allows array attributes for services tags since 6.2: symfony/symfony#47364

@franmomu franmomu added this to the 4.6.0 milestone Mar 14, 2023
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a minor comment on named args.

Comment on lines 427 to 437
$container->getDefinition('doctrine_mongodb.odm.entity_value_resolver')->setArgument(2, (new Definition(MapEntity::class))->setArguments([
null,
null,
null,
$controllerResolverDefaults['mapping'] ?? null,
null,
null,
null,
null,
$controllerResolverDefaults['disabled'] ?? false,
]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the container support named arguments? That might make this a little easier to read.

<argument type="service" id="doctrine_mongodb" />
<argument type="service" id="doctrine_mongodb.odm.entity_value_resolver.expression_language" on-invalid="ignore" />
<tag name="controller.argument_value_resolver" priority="110">
<attribute name="name">Symfony\Bridge\Doctrine\ArgumentResolver\EntityValueResolver</attribute>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be:
<tag name="Symfony\Bridge\Doctrine\ArgumentResolver\EntityValueResolver" priority="110">controller.argument_value_resolver</tag>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually NO: this name would conflict with https://github.com/doctrine/DoctrineBundle/blob/2.9.x/Resources/config/orm.xml#L194 so we need to find another one

Copy link
Member

@nicolas-grekas nicolas-grekas Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second though, this name might be the right one. It should allow #[MapEntity] to work correctly. I don't know how one would expect this to resolve the conflict when there is both ORM + MongoDB but I'd be tempted to say this should be handled inside EntityValueResolver (it could decide on its own which manager to use.)

Copy link
Contributor Author

@franmomu franmomu Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC using Symfony 6.3 (with pinned resolvers), #[MapEntity] extending ValueResolver will set EntityValueResolver::class as resolver, but there won't be any resolver named EntityValueResolver::class, even if you are only using ORM, there will be doctrine.orm.entity_value_resolver but not EntityValueResolver::class, so using #[MapEntity] without specifying the resolver will probably fail?

Update: now I see that the ORM will also set its doctrine.orm.entity_value_resolver with name EntityValueResolver and there is when the conflict will happen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolver it registered under its FQCN, see name attribute on the tag in the XML I linked above.

Comment on lines +426 to +429
$mapEntityDefinition = $container->getDefinition('doctrine_mongodb.odm.entity_value_resolver.map_entity');

$mapEntityDefinition->setArgument('$mapping', $controllerResolverDefaults['mapping'] ?? null);
$mapEntityDefinition->setArgument('$disabled', $controllerResolverDefaults['disabled'] ?? false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alcaeus I've registered the service doctrine_mongodb.odm.entity_value_resolver.map_entity previously and this is another way to only set some parameters, not sure if this is recommended though since if the name of the argument is changed, this will fail.

@franmomu
Copy link
Contributor Author

We are kind of stuck in here, the problem is that with targeted value resolvers, MapEntity is tied (by FQCN) to EntityValueResolver so we cannot register another one (symfony/symfony#49905)

In our side, the only thing I can think of is to create a DocumentValueResolver class which would decorate EntityValueResolver and create a MapDocument class which would extend MapEntity.

@franmomu franmomu changed the title Leverage pinned resolvers Leverage targeted resolvers May 15, 2023
@boskos-q
Copy link

In our side, the only thing I can think of is to create a DocumentValueResolver class which would decorate EntityValueResolver and create a MapDocument class which would extend MapEntity.

@franmomu imo this would make the most sense to do

@franmomu franmomu deleted the add_name_tag branch July 26, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage pinned resolvers
4 participants