Skip to content

Don't mention the UserInterface type-hinting #9060

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

Closed
wants to merge 3 commits into from

Conversation

javiereguiluz
Copy link
Member

This fixes #7506. Ping @iltar.

}

.. tip::

The user will be an object and the class of that object will depend on
your :ref:`user provider <security-user-providers>`.

.. versionadded:: 3.2
The ability to get the user by type-hinting an argument with UserInterface
was introduced in Symfony 3.2.
Copy link
Contributor

@linaori linaori Jan 15, 2018

Choose a reason for hiding this comment

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

@javiereguiluz
Copy link
Member Author

My changes are wrong because UserInterface is used in other parts of the article. We need to make a definitive decision about UserInterface:

a) Never use it or mention on Symfony Docs
b) Never use it, but mention it in a note/tip somewhere, but discourage to use it
c) Always use it, and explain the $this->getUser() shortcut in a note/tip somewhere, but discourage to use it

I thought we were going for (a).

Ping @weaverryan @chalasr @xabbuh

@xabbuh
Copy link
Member

xabbuh commented Jan 16, 2018

I am not sure I follow what you would like to fix or at which stage we are right now.

@javiereguiluz
Copy link
Member Author

@xabbuh we introduced UserInterface as a replacement of $this->getUser() ... but apparently using UserInterface is bad and even Ryan asked to keep using $this->getUser().

So, we need to make a definitive decision about UserInterface. Is something we want to promote or not? Thanks!

@linaori
Copy link
Contributor

linaori commented Jan 16, 2018

UserInterface $user provides an alternative for people that do not (want to) extend the Controller or implement the trait. If there is supposed to 1 way "to rule them all", it should be one that works for both.

@javiereguiluz
Copy link
Member Author

@iltar if we do that, it'd be better if we type-hint the new Security utility class instead of UserInterface. Then we can do $security->getUser()

@linaori
Copy link
Contributor

linaori commented Jan 16, 2018

That would be an extra action and an extra dependency, which would beat the purpose imo

@weaverryan
Copy link
Member

UserInterface $user provides an alternative for people that do not (want to) extend the Controller or implement the trait.

This is definitely true! :) But we don't need to give this group very much help - it's a very small and very intelligent group.

The problem with type-hinting is that it's not "discoverable": I can't think about my code or dig around and realize that UserInterface is a valid thing to type-hint. We already allow type-hinting for services and we added debug:autowiring so that (at least) there's an easy place to go to find the list of things you can type-hint. I don't think we should recommend any other type-hints (yes, we also have Request and param converters... so plenty already).

I still think we should go with $this->getUser() everywhere: let's not complicate people's lives, this is a really easy way to get the user :). Also, I prefer to show the Security class a secondary option, instead of UserInterface, because you can inject Security elsewhere (so that code will work anywhere, not just in a controller).

I know we disagree on this @iltar :). But the DX looks bad for UserInterface vs $this->getUser(), and for smart users like you, you don't need the docs to help you (other than a mention of this way).

@linaori
Copy link
Contributor

linaori commented Jan 18, 2018

As long as the feature is documented somewhere, I'm 👍 Would be a bit troublesome to have undocumented features, might as well remove them in that case.

@weaverryan
Copy link
Member

@javiereguiluz can you update you the PR - let's look for all UserInterface spots and make them getUser(), but leave a mention somewhere.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

For the record, I do love the security user value resolver and use it over the controller shortcut, but I can understand that the shortcut is preferred for the docs.

javiereguiluz added a commit that referenced this pull request Jan 20, 2018
This PR was squashed before being merged into the 3.3 branch (closes #9060).

Discussion
----------

Don't mention the UserInterface type-hinting

This fixes #7506. Ping @iltar.

Commits
-------

3347569 Don't mention the UserInterface type-hinting
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.

7 participants