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

Added docs about ArgumentValueResolvers #6438

Closed

Conversation

linaori
Copy link
Contributor

@linaori linaori commented Apr 6, 2016

Q A
Doc fix? no
New docs? yes
Applies to 3.1
Fixed tickets ~

Adds the documentation for the new ArgumentValueResolver feature from symfony/symfony#18308.

action in the forementioned functionality.

- The :class:``Symfony\\Component\\HttpKernel\\ControllerMetadata\\ArgumentMetadata`` represents
information retrieved from the method signature for the current argument it's trying to resolve.
Copy link
Member

Choose a reason for hiding this comment

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

Lists shouldn't be indented with one space in reSt, it'll end up as a blockquote. Also, please use * to be consistent with the other lists in the documentation.

@linaori linaori force-pushed the feature/argument-value-resolvers branch from a1d55d2 to a310bbc Compare April 6, 2016 15:10
The ``ArgumentResolver`` and value resolvers are added in Symfony 3.1.

In the book, you've learned that you can add the :class:`Symfony\\Component\\HttpFoundation\\Request`
as action argument and it will be injected into the method. This is done via the
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "In the book, you've learned that you can get the Request object by adding a Request argument to your controller."?

@wouterj
Copy link
Member

wouterj commented Apr 6, 2016

Thank you for writing the docs, @iltar! It's looking great. I'm afraid I've decorated your PR a bit with some comments, but they are mostly minor style comments.


Prior to Symfony 3.1, this logic was resolved within the ``ControllerResolver``. The old
functionality is moved to the ``LegacyArgumentResolver``, which contains the previously
used resolving logic.
Copy link
Member

Choose a reason for hiding this comment

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

If #18495 is merged, the legacy class won't exist anymore.

@linaori
Copy link
Contributor Author

linaori commented Apr 11, 2016

@fabpot in the example I'm writing the UserValueResolver which I've based on the PR in the SensioFrameworkExtraBundle. While we're at the subject, should I add this code to a PR for 3.1, wait for 3.2 or is this something specific enough that people should implement them self?

@fabpot
Copy link
Member

fabpot commented Apr 11, 2016

I think it makes sense to add this feature directly in Symfony... that's one reason we made things more flexible. Submit this to master, that could be merged into 3.1 or 3.2.

@linaori linaori force-pushed the feature/argument-value-resolvers branch from fdce9c6 to 55a87b5 Compare April 29, 2016 13:51
@linaori
Copy link
Contributor Author

linaori commented Apr 29, 2016

I've rebased against the current master and added the fixes in 55a87b5

is resolved, you must `yield`_ the value to the ``ArgumentResolver``.

Both methods get the ``Request`` object, which is the current request, and an
:class:`Symfony\\Component\\HttpKernel\\ControllerMetadata\\ArgumentMetadata`.
Copy link
Member

Choose a reason for hiding this comment

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

I would add "instance" in this sentence: "Both methods get [...] and an ArgumentMetadata instance"

@linaori
Copy link
Contributor Author

linaori commented May 6, 2016

Any last checks? I'd say this is ready to be merged 😄

@xabbuh xabbuh added this to the 3.1 milestone May 14, 2016
The ``ArgumentResolver`` and value resolvers were introduced in Symfony 3.1.

In the book, you've learned that you can get the :class:`Symfony\\Component\\HttpFoundation\\Request`
object via an argument in your controller. This argument has to be typehinted
Copy link
Member

Choose a reason for hiding this comment

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

type-hinted

@xabbuh
Copy link
Member

xabbuh commented May 21, 2016

This looks ready to me. 👍

wouterj added a commit that referenced this pull request Jun 11, 2016
This PR was submitted for the master branch but it was merged into the 3.1 branch instead (closes #6438).

Discussion
----------

Added docs about ArgumentValueResolvers

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | 3.1
| Fixed tickets | ~

Adds the documentation for the new `ArgumentValueResolver` feature from symfony/symfony#18308.

Commits
-------

f22dc96 Added docs about ArgumentValueResolvers
@wouterj
Copy link
Member

wouterj commented Jun 11, 2016

Thank you @iltar! I've merged your PR in the 3.1 branch.

I've proofread the article and fixed some class names, typos. I've also removed the section about optional value resolvers and replaced it with a tip directive. I don't think it's needed to use so much words to explain it.

If you have some time, please review the changes in 66ce882 (please note that I also line wrapped some paragraphs, the diff looks way bigger than the actual changes).

@wouterj wouterj closed this Jun 11, 2016
@linaori
Copy link
Contributor Author

linaori commented Jun 12, 2016

@wouterj I've found 2 small issues but that's about it, looks good to me!

xabbuh added a commit that referenced this pull request Jun 20, 2016
This PR was merged into the 3.1 branch.

Discussion
----------

[#6438] some tweaks to the argument value resolver

Commits
-------

2cdd6dc [#6438] some tweaks to the argument value resolver
fabpot added a commit to symfony/symfony that referenced this pull request Jul 1, 2016
This PR was merged into the 3.2-dev branch.

Discussion
----------

Added a SecurityUserValueResolver for controllers

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

This PR uses the new `ArgumentResolver` to inject a security user when the signature implies so. This is based on the [docs code example](symfony/symfony-docs#6438 (comment)) and [existing pr on the SFEB](sensiolabs/SensioFrameworkExtraBundle#327).

With the new example you can do the following:
```php
// when a User is mandatory, e.g. behind firewall
public function fooAction(UserInterface $user)

// when a User is optional, e.g. where it can be anonymous
public function barAction(UserInterface $user = null)
```
This deprecates the `Controller::getUser()` method.

I have added it on a priority of 40 so it falls just under the `RequestValueResolver`. This is because it's already used and the initial performance is less of an impact.

There was a comment asking if the `controller_argument.value_resolver` tag name wasn't too long. If decided this tag should change before 3.1 is released, I will update it in here.

*`RequestValueResolver` contains a small codestyle consistency fix.*

Commits
-------

d341889 Added a SecurityUserValueResolver for controllers
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

8 participants