Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

UserValueResolver and SecurityUserValueResolver improvement#26971

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
davidkmenta wants to merge3 commits intosymfony:masterfromdavidkmenta:better-user-value-resolver
Closed

UserValueResolver and SecurityUserValueResolver improvement#26971

davidkmenta wants to merge3 commits intosymfony:masterfromdavidkmenta:better-user-value-resolver

Conversation

@davidkmenta
Copy link

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

UserValueResolver and SecurityUserValueResolver should also resolve class implementing UserInterface.

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

this was on my list :)

{
// only security user implementations are supported
if (UserInterface::class !==$argument->getType()) {
if (!$argument->getType() || !$this->implementsCorrectInterface($argument->getType())) {
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 inSymfony\Component\Security\Http\Controller\UserValueResolver instead (see deprecation notice above)

Copy link
Author

Choose a reason for hiding this comment

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

It's in both classes :)

Copy link
Member

Choose a reason for hiding this comment

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

Deprecated classes are frozen until they are suppressed, it should not get new features.

*/
privatefunctionimplementsCorrectInterface($type)
{
return UserInterface::class ===$type ||array_key_exists(UserInterface::class,class_implements($type));
Copy link
Member

Choose a reason for hiding this comment

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

Let's inline this logic in the calling condition

Choose a reason for hiding this comment

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

isn't this whatis_a() oris_subclass_of() does?

chalasr, ogizanagi, davidkmenta, and jvasseur reacted with thumbs up emoji
Copy link
Author

Choose a reason for hiding this comment

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

Didn't know 😳thanks!

@linaori
Copy link
Contributor

Discussion why this feature wasn't introduced when initially adding the resolver:#18510 (comment)

@nicolas-grekasnicolas-grekas added this to the4.2 milestoneApr 18, 2018
@davidkmenta
Copy link
Author

@iltar thanks for the link. I agree that it may collide with the ParamConverter. On the other hand I still think the UserValueResolver should support classes implementing the UserInterface.

Few quick ideas:

  • What about making it configurable or togglable?
  • Is a ParamConverter called before or after a ValueResolver? Or not at all?

@jvasseur
Copy link
Contributor

jvasseur commentedApr 19, 2018
edited
Loading

I agree with@davidkmenta here, the param converters are executed before the value resolver so the concern highlighted in#18510 (comment) shouldn't be a problem.

Forcing users to type they controller arguments withUserInterface gives bad DX for IDE auto-completion/static analysis.

@linaori
Copy link
Contributor

@davidkmenta@jvasseur If you type-hint an entity, it will actually try to inject that entity based on the request, which will fail, because there's no way of fetching it. This isbecause the parameter converters are triggered first.

Forcing users to type they controller arguments withUserInterface gives bad DX for IDE auto-completion/static analysis.

If people use it as quick way to get their current user object, why not provide a (by the SecurityBundle) configurable AVR that can return a given object based on the currentUserInterface? That would be a generic solution to a common problem.

@jvasseur
Copy link
Contributor

@iltar didn't think of that since I tend do disable the automatic conversion.

We need to think of the future of the future of param converters (sensiolabs/SensioFrameworkExtraBundle#436 ?) maybe it's time to deprecate them and introduce configurable argument resolvers, this would allow us to have a configurable security user resolver.

@linaori
Copy link
Contributor

Yes,something has to be done about the parameter converters, but I'm not sure what and I don't have much time myself 😢

Regarding the current AVR, what if we add an additional interface to theUserProviderInterface?

interface DomainUserProviderInterface{// would return your entity for example, names to be discussedpublicfunctionconvertToDomainUser(UserInterface$user);}
# follows the basic configurationsecurityproviders:hostnet:id:App\Security\Authentication\CustomerPortal\UserProvider

This would make it possible for the AVR to resolve the security user to a domain user. If you implement the interface on your user provider, it will simply be "enabled" for whatever has that user. We have the firewall information available, so perhaps it's not even hard to find the user provider.

@davidkmenta
Copy link
Author

So I guess this PR can be closed for now :)

@gmponos
Copy link
Contributor

TBH I do not follow why this was closed because of bundle that it's not inside symfony.. Personally I do not use Sensio bundles... :(

I believe that this is a problem of sensio bundle that should either decorate the user AVR or overwrite it.

@linaori
Copy link
Contributor

Symfony needs a generic controller configuration system that works similar to the idea behind the annotations from that bundle (but configurable without annotations as well). When this is done, AVR can fully replace the parameter converters and it won't be an issue anymore.

The issue arises becausea lot of developersdo use theSensioFrameworkExtraBundle, because it also adds features such as@Security. There's also effort being made to deprecate the user object from the Token, so the idea is to be able to inject your Domain user into an action based on this token, which means this AVR wouldn't even be necessary anymore. This won't solve the aforementioned issue though.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@chalasrchalasrchalasr left review comments

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

7 participants

@davidkmenta@linaori@jvasseur@gmponos@nicolas-grekas@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp