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

[Security/Http] Make UserValueResolver accept any subtype of UserInterface#30401

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
Pierstoval wants to merge1 commit intosymfony:masterfromPierstoval:user-value-resolver-subtypes
Closed

[Security/Http] Make UserValueResolver accept any subtype of UserInterface#30401

Pierstoval wants to merge1 commit intosymfony:masterfromPierstoval:user-value-resolver-subtypes

Conversation

@Pierstoval
Copy link
Contributor

@PierstovalPierstoval commentedFeb 27, 2019
edited
Loading

QA
Branch?master/4.3
Bug fix?no
New feature?yes
BC breaks?no (not sure)
Deprecations?no
Tests pass?yes
LicenseMIT

With a controller we can inject theUserInterface object like this:

publicfunctionindex(Request$request,UserInterface$user =null):Response{// ...}

However, this has a drawback: it hides thedomain user for actions that need the right type-hint, else, it means we're "blind", which means that we must addif ($user instanceof MyUserObject) { /* ... */} to execute our logic.

The goal of this PR is to allow this:

useApp\Entity\User;// ...publicfunctionindex(Request$request,User$user =null):Response{// ...}

This is just a basic suggestion that is quite straightforward (so if it's not suitable, feel free to tell me, if you know the component better than me 👍 )!

If it's not possible, I also thought about creating aSecurityTokenValueResolver, also probably straightforward 👍 (will probably work on a separate PR for this)

onEXHovia, dmaicher, and gmponos reacted with thumbs up emoji
@chalasr
Copy link
Member

Already thought about this, but was unsure. ping@linaori

@chalasrchalasr added this to thenext milestoneFeb 28, 2019
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.

Could you please add a note in the Security CHANGELOG?

// only security user implementations are supported
if (UserInterface::class !==$argument->getType()) {
$type =$argument->getType();
if (UserInterface::class !==$type && !is_subclass_of($type, UserInterface::class)) {
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 don't get new features, should be reverted

// only security user implementations are supported
if (UserInterface::class !==$argument->getType()) {
$type =$argument->getType();
if (UserInterface::class !==$type && !is_subclass_of($type, UserInterface::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if l48 should be changed toreturn $user instanceof $type to prevent a type error?

// only security user implementations are supported
if (UserInterface::class !==$argument->getType()) {
$type =$argument->getType();
if (UserInterface::class !==$type && !is_subclass_of($type, UserInterface::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please test this with a parameter converter to see if this will work as expected when the UserInterface is an entity and parameter converters are enabled.

jvasseur and Koc reacted with thumbs up emoji
@Koc
Copy link
Contributor

Koc commentedMar 3, 2019

This was proposed multiple times ago (#18510 ,#26971) and was rejected due to reasons described in this comment#18510 (comment)

@chalasr
Copy link
Member

Now that we have#30914 I think we can close here.

@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@PierstovalPierstoval deleted the user-value-resolver-subtypes branchJune 10, 2020 08:17
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@chalasrchalasrchalasr requested changes

+1 more reviewer

@linaorilinaorilinaori left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@Pierstoval@chalasr@Koc@linaori@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp