8000 Add an Argument Resolver by jderusse · Pull Request #1413 · doctrine/DoctrineBundle · GitHub
[go: up one dir, main page]
Skip to content

Add an Argument Resolver #1413

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add an Argument Resolver #1413

wants to merge 1 commit into from

Conversation

jderusse
Copy link
Contributor
@jderusse jderusse commented Oct 26, 2021 •
edited
Loading

This PR migrates the SensioFrameworkExtraBundle's Doctrine ParamsConverter to Argument Resolver.

Moving away from ParamConverter would fix symfony/symfony#40333

usage:
enabled the behavior

doctrine:
    dbal:
        default_connection: default
        connections:
            default:
                dbname: db

    orm:
        argument_resolver: true

The default configuration works out of the box. The entity is fetch using $repo->find() using the route's attribute.

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

Options can be configured with a new Attribute

use Doctrine\Bundle\DoctrineBundle\Attribute\Entity;

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

note: the business logic and tests are copy/pasted from the original repo. Tell me if you think some options are not needed anymore.

jderusse
jderusse commented Oct 26, 2021
View reviewed changes
@jderusse jderusse force-pushed the argument-resolver branch 2 times, most recently from feab3e4 to cc32106 Compare October 26, 2021 19:56
@ostrolucky ostrolucky changed the base branch from 2.4.x to 2.5.x October 26, 2021 19:57
@jderusse jderusse force-pushed the argument-resolver branch 5 times, most recently from b9b1123 to a63a8a0 Compare October 26, 2021 21:20
@jderusse jderusse changed the title Add a Argument Resolver Add an Argument Resolver Oct 27, 2021
jvasseur
jvasseur reviewed Oct 27, 2021
View reviewed changes
stof
stof reviewed Oct 27, 2021
View reviewed changes
ArgumentResolver/EntityValueResolver.php
{
if (! $options['mapping']) {
$keys = $request->attributes->keys();
$options['mapping'] = $keys ? array_combine($keys, $keys) : [];
Copy link
Member

Choose a reason for hiding this comment

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

This default mapping that takes all request attributes to find the entity is something that I'd like to make configurable (through the config, not in each argument), as it is quite confusing. I had several bugs in my project where an optional argument was suddenly given a weird value instead of null just because the controller action had another argument with a name matching the name of an association (and that this default mapping would silently use it to build a criteria)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

providing a semantic configuration that will change the default behavior is something that could/should be done in a dedicated PR IMHO.

Maybe related to #1413 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to disable the whole argument resolver. I want a way to disable the "use any other request param as a lookup" behavior. In my project, I never found a case where I wanted it, and I got several bugs caused by it for optional parameters of controllers.
Cases that actually want to do a lookup by another argument than the current argument will often need to provide a mapping explicitly anyway, as they expect to use a given field, not "any field" either (maybe they are just lucky that the names of their other controller arguments don't match fields that exist in their entity today, and they won't add new fields or new arguments in the future that would break that lucky invariant)

Copy link

Choose a reason for hiding this comment

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

Making this configurable globally would means bundles won't be able to rely on one the the behaviors. My take on this would be to completely remove the feature as something that is to much magic for an argument resolver.

Copy link
Member

Choose a reason for hiding this comment

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

well, I'm also fine with removing the magic mapping entirely too (the mapping option should still exist for cases where it is configured explicitly, as it solves a use case)

Copy link
Contributor Author
@jderusse jderusse Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

id and the attribute with the same name than the argument will both use the method find . here we are talking about the fallback that use attributes that match none of the 2 cases above.
Disabling this feature globally does not make sens. If the request does not contain id nor an attribute with the same, then there will be nothing to match, entity could not be found, and user would have to provide a mapping on every attributes anyway.

Sorry, I disagree with you

/blogs/{slug} or /product/{uuid} just work, and is often used. No need for custom mapping in such cases.

At the opposite, I never had issue with route containing several attributes. maybe I'm lucky, or because routes with multiple arguments are rare enough to provide mapping or expr and are not ambiguous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anyway, I added a configuration section and added an auto_mapping flag that allows users to disable this behavior. This will enforce people to either use {id} (or the same name as the argument) OR to explicitly provide a mapping option (or id or expr).

@mbabker
Copy link
Contributor
mbabker commented Oct 30, 2021

Maybe the resolver and attribute should go to the Doctrine Bridge instead of each bundle? Based on sensiolabs/SensioFrameworkExtraBundle#744 it seems like this could work OK for more than just the ORM.

@jderusse jderusse closed this Oct 30, 2021
@jderusse jderusse reopened this Oct 30, 2021
@jderusse
Copy link
Contributor Author
jderusse commented Oct 30, 2021 •
edited
Loading

Maybe the resolver and attribute should go to the Doctrine Bridge instead of each bundle? Based on sensiolabs/SensioFrameworkExtraBundle#744 it seems like this could work OK for more than just the ORM.

What do you mean by "each bundle". the PR you are referring is about ParamConverter and not Argument Resolver.

This PR is about making the glue between Symfony and the ORM. The bundle sounds an appropriate place for such code.

Edit sorry for multiple close/open currently typing on phone

@jderusse jderusse reopened this Oct 30, 2021
@mbabker
Copy link
Contributor
mbabker commented Oct 30, 2021

Based on how that PR reads, it sounds like the original param converter code would work just fine for any object manager (be it the ORM or any of the ODM implementations). So instead of this bundle, the MongoDB bundle, and the PHPCR bundle needing to re-implement basically the same class, they could all use one common class from the Doctrine Bridge and the bundles would just be left to handle the configuration bits.

@ostrolucky
Copy link
Member

I agree. I would rather this was in doctrine-bridge.

@roukmoute roukmoute mentioned this pull request Nov 2, 2021
Merged
4 tasks
@ostrolucky
Copy link
Member

as mentioned, class should be in bridge and bundle will only provide wiring

@ostrolucky ostrolucky closed this Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@jvasseur jvasseur jvasseur left review comments

Assignees
No one assigned
Labels
Status: Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

#[CurrentUser] : Unable to guess how to get a Doctrine instance from the request information for parameter "user".
5 participants
@jderusse @mbabker @ostrolucky @stof @jvasseur
0