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

[SecurityBundle] Set UserValueResolver's priority higher than EntityValueResolver #48032

Merged
merged 1 commit into from Nov 9, 2022

Conversation

kbond
Copy link
Member

@kbond kbond commented Oct 28, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

UserValueResolver's priority is currently 40 and EntityValueResolver's priority is 110 (configured in doctrine-bundle).

Currently, to use the CurrentUser attribute and MapEntity (when auto_mapping is enabled), you need to do the following to have it work:

public function postAction(
  #[CurrentUser]
  #[MapEntity(disabled: true)]
  User $user,
  Post $post
)

This removes this need for #[MapEntity(disabled: true)] but I'm not sure the larger impact of increasing the priority of UserValueResolver. Here is some context as to why the priorities are they way they are:

@carsonbot
Copy link

Hey!

I think @johnkrovitch has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

@stof
Copy link
Member

stof commented Nov 9, 2022

The impact would be that if you have a parameter typehinted as UserInterface or tagged as CurrentUser that has the same name than a request attribute, the UserValueResolver would now apply instead of the RequestAttributeResolver. But this is more likely to fix bugs (or at least fix DX confusion) than to introduce it (if you tag as CurrentUser, you make the intent clear that you expect the current user and not the request attribute named user)

@nicolas-grekas
Copy link
Member

Thank you @kbond.

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

Successfully merging this pull request may close these issues.

None yet

4 participants