Documented the ArgumentResolver along the ControllerResolver #6422

linaori · 2016-04-01T11:33:49Z

Q	A
Doc fix?	yes
New docs?	no ~ symfony/symfony#18308
Applies to	3.1
Fixed tickets	~

The ArgumentResolver is used now instead of the ControllerResolver. I have yet to document the extension point but first I want to have this page mention it.

javiereguiluz · 2016-04-01T13:48:52Z

@iltar thanks for this contribution. We'll need to change a bit the "tone" (some parts read too informal) but so far contents are very promising. Thanks!

linaori · 2016-04-01T13:52:46Z

@javiereguiluz sure thing! I'm not too experienced with the language style so I'm firstly making sure the contents is up-to-date so it can be fine-tuned afterwards. This is always the hardest part for me writing documentation.

When writing code, you have codestyle, but when you're writing texts it's almost freestyle ;)

javiereguiluz · 2016-04-01T13:55:25Z

@iltar you are doing great! It's just a matter of minor "cosmetic changes". But for now don't worry about them 😄

linaori · 2016-04-01T14:00:25Z

@javiereguiluz is the array shorthand notation preferred or do you want the full notation?

edit - for now I will keep amending so I can see if I make parse errors without too many issues, phpstorm only shows me highlights, no validation

javiereguiluz · 2016-04-01T14:14:49Z

@iltar we still use array() in the docs.

fabpot · 2016-04-03T09:19:09Z

components/http_kernel/introduction.rst

+
+.. caution::
+
+    The `getArguments()` method in the :class:`Symfony\\Component\\Httpkernel\\Controller\\ControllerResolver`


getArguments must be surrounded by two `

@wouterj

…iltar, HeahDude) This PR was merged into the 3.1-dev branch. Discussion ---------- Added an ArgumentResolver with clean extension point | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #17933 (pre-work), #1547, #10710 | License | MIT | Doc PR | symfony/symfony-docs#6422 **This PR is a follow up for and blocked by: #18187**, relates to #11457 by @wouterj. When reviewing, please take the last commit: [Added an ArgumentResolver with clean extension point](4c092b3) This PR provides: - The ability to tag your own `ArgumentValueResolverInterface`. This means that you can effectively expand on the argument resolving in the `HttpKernel` without having to implement your own `ArgumentResolver`. - The possibility to cache away argument metadata via a new `ArgumentMetadataFactory` which simply fetches the data from the cache, effectively omitting 1 reflection call per request. *Not implemented in this PR, but possible once this is merged.* - The possibility to add a PSR-7 adapter to resolve the correct request, avoids the paramconverters - The possibility to add a value resolver to fetch stuff from $request->query - Drupal could simplify [their argument resolving](https://github.com/drupal/drupal/blob/8.1.x/core/lib/Drupal/Core/Controller/ControllerResolver.php) by a lot - etc. The aim for this PR is to provide a 100% BC variant to add argument resolving in a clean way, this is shown by the 2 tests: `LegacyArgumentResolverTest` and `ArgumentResolverTest`. /cc @dawehner @larowlan if you have time, can you check the impact for Drupal? I think this should be a very simple change which should make it more maintainable. Commits ------- 1bf80c9 Improved DX for the ArgumentResolver f29bf4c Refactor ArgumentResolverTest cee5106 cs fixes cfcf764 Added an ArgumentResolver with clean extension point 360fc5f Extracting arg resolving from ControllerResolver

linaori · 2016-04-06T13:02:40Z

So the PR is merged and the previous comments are addressed. I will open a new PR once I'm done writing some initial docs about the actual feature.

I think everything code related is fixed in this PR so it's up to the text itself now

xabbuh · 2016-04-06T16:18:47Z

@iltar Is this supposed to be merged before #6438 or the other way around?

linaori · 2016-04-06T17:06:38Z

@xabbuh it doesn't really matter, one describes the feature, the other makes sure the documentation doesn't trigger deprecations in your code.

HeahDude · 2016-04-06T22:17:18Z

components/http_kernel/introduction.rst

@@ -85,11 +85,22 @@ is really simple and involves creating an
 :doc:`event dispatcher </components/event_dispatcher/introduction>` and a
 :ref:`controller resolver <component-http-kernel-resolve-controller>` (explained
 below). To complete your working kernel, you'll add more event listeners
-to the events discussed below::
+to the events discussed below


missing colon ?

Moved the caution to below, this also fixes the issue where the : refers to a caution instead of the intended code.

Then it should be a full stop (.).

This PR was merged into the 3.1-dev branch. Discussion ---------- [HttpKernel] Renamed the argument resolver tag | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | not if merged before 3.1 | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Changed as discussed several times: #18510 (comment), symfony/symfony-docs#6422 (comment). Commits ------- cd10057 Renamed argument resolver tag, added test

This PR was merged into the 3.1-dev branch. Discussion ---------- [HttpKernel] Renamed the argument resolver tag | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | not if merged before 3.1 | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Changed as discussed several times: symfony/symfony#18510 (comment), symfony/symfony-docs#6422 (comment). Commits ------- cd10057 Renamed argument resolver tag, added test

linaori · 2016-04-15T10:08:50Z

Broken build is related to:

:doc:`/cookbook/controller/argument_value_resolver`

This will be solved as soon as #6438 is merged.

wouterj · 2016-06-11T20:00:09Z

Merged #6438 now. Can you please rebase?

linaori · 2016-06-12T10:01:49Z

@wouterj rebase is done and checks pass now

wouterj · 2016-06-12T10:16:00Z

components/http_kernel/introduction.rst

+:class:`Symfony\\Component\\HttpKernel\\Controller\\ControllerResolverInterface` and
+use any argument resolver that implements the
+:class:`Symfony\\Component\\HttpKernel\\Controller\\ArgumentResolverInterface`.
+However, the HttpKernel component comes with some built-in listeners, and everything


remove the comma before and

wouterj · 2016-06-12T10:19:35Z

Can you please also fix the outstanding comments (both from me and others)? Sorry for not reviewing it before asking you to rebase.

xabbuh · 2016-07-04T07:31:28Z

@iltar Is this still a work in progress from your point of view?

linaori · 2016-07-04T07:39:42Z

@xabbuh I totally forgot to fix the title! No, this should be finished.

xabbuh · 2016-07-04T11:57:13Z

create_framework/separation_of_concerns.rst

    $response = $framework->handle($request);

    $response->send();

+


should be removed

xabbuh · 2016-07-04T11:57:34Z

👍

linaori · 2016-07-04T12:08:38Z

Fixed a merge conflict and the feedback, can you please check it 1 time if everything went fine? I had a small issue inbetween

xabbuh · 2016-07-05T08:44:21Z

👍 still looks good to me

Status: Reviewed

Note to merger: This needs to be merged in the 3.1 branch.

…olver (iltar) This PR was submitted for the master branch but it was merged into the 3.1 branch instead (closes #6422). Discussion ---------- Documented the ArgumentResolver along the ControllerResolver | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no ~ symfony/symfony#18308 | Applies to | 3.1 | Fixed tickets | ~ The ArgumentResolver is used now instead of the ControllerResolver. I have yet to document the extension point but first I want to have this page mention it. Commits ------- 11920e3 Documented the ArgumentResolver along the ControllerResolver

wouterj · 2016-07-05T10:07:20Z

Thanks @iltar! This is a big PR :)

I've merged this into the 3.1 branch as usual. After merging, I reread all changed sections and did some minor changes in 998cc40 It would be great if you can review them.

linaori · 2016-07-05T10:10:19Z

@wouterj I've checked it, LGTM

linaori force-pushed the feature/argument-resolvers branch 3 times, most recently from 00e0e7b to 678c705 Compare April 1, 2016 11:56

linaori changed the title ~~Documented the ArgumentResolver along the ControllerResolver~~ [WiP] Documented the ArgumentResolver along the ControllerResolver Apr 1, 2016

linaori force-pushed the feature/argument-resolvers branch 2 times, most recently from bd2fb26 to 947626c Compare April 1, 2016 13:47

linaori force-pushed the feature/argument-resolvers branch 2 times, most recently from 25f4985 to 2eecbd2 Compare April 1, 2016 14:21

linaori mentioned this pull request Apr 2, 2016

Added an ArgumentResolver with clean extension point symfony/symfony#18308

Merged

linaori force-pushed the feature/argument-resolvers branch 2 times, most recently from 66ee94b to 1ce0cc7 Compare April 2, 2016 16:28

fabpot reviewed Apr 3, 2016
View reviewed changes

xabbuh added the Status: Needs Work label Apr 3, 2016

HeahDude reviewed Apr 6, 2016
View reviewed changes

javiereguiluz mentioned this pull request Apr 12, 2016

Added a SecurityUserValueResolver for controllers symfony/symfony#18510

Merged

linaori mentioned this pull request Apr 13, 2016

[HttpKernel] Renamed the argument resolver tag symfony/symfony#18529

Merged

linaori force-pushed the feature/argument-resolvers branch from 24b11e2 to a9897c7 Compare April 14, 2016 11:30

xabbuh added the On hold label Apr 15, 2016

wouterj removed the On hold label Jun 11, 2016

linaori force-pushed the feature/argument-resolvers branch from a9897c7 to 4edfe5a Compare June 12, 2016 09:13

wouterj reviewed Jun 12, 2016
View reviewed changes

linaori changed the title ~~[WiP] Documented the ArgumentResolver along the ControllerResolver~~ Documented the ArgumentResolver along the ControllerResolver Jul 4, 2016

xabbuh reviewed Jul 4, 2016
View reviewed changes

create_framework/separation_of_concerns.rst

$response = $framework->handle($request);

$response->send();

Copy link

Member

xabbuh Jul 4, 2016

Choose a reason for hiding this comment

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

should be removed

linaori force-pushed the feature/argument-resolvers branch from a26f622 to 3eda91d Compare July 4, 2016 12:06

Documented the ArgumentResolver along the ControllerResolver

f630d8d

linaori force-pushed the feature/argument-resolvers branch from 3eda91d to f630d8d Compare July 4, 2016 12:08

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jul 5, 2016

wouterj added a commit that referenced this pull request Jul 5, 2016

[#6422] Detailed proof-read after the great PR from @iltar

998cc40

wouterj closed this Jul 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documented the ArgumentResolver along the ControllerResolver #6422

Documented the ArgumentResolver along the ControllerResolver #6422

linaori commented Apr 1, 2016

javiereguiluz commented Apr 1, 2016

linaori commented Apr 1, 2016

javiereguiluz commented Apr 1, 2016

linaori commented Apr 1, 2016

javiereguiluz commented Apr 1, 2016

fabpot Apr 3, 2016

linaori commented Apr 6, 2016

xabbuh commented Apr 6, 2016

linaori commented Apr 6, 2016

HeahDude Apr 6, 2016

linaori Apr 7, 2016

wouterj Jun 12, 2016

linaori commented Apr 15, 2016 •

edited

Loading

wouterj commented Jun 11, 2016

linaori commented Jun 12, 2016

wouterj Jun 12, 2016

wouterj commented Jun 12, 2016

xabbuh commented Jul 4, 2016

linaori commented Jul 4, 2016

xabbuh Jul 4, 2016

xabbuh commented Jul 4, 2016

linaori commented Jul 4, 2016

xabbuh commented Jul 5, 2016

wouterj commented Jul 5, 2016

linaori commented Jul 5, 2016


		.. caution::

		The `getArguments()` method in the :class:`Symfony\\Component\\Httpkernel\\Controller\\ControllerResolver`

Documented the ArgumentResolver along the ControllerResolver #6422

Documented the ArgumentResolver along the ControllerResolver #6422

Conversation

linaori commented Apr 1, 2016

javiereguiluz commented Apr 1, 2016

linaori commented Apr 1, 2016

javiereguiluz commented Apr 1, 2016

linaori commented Apr 1, 2016

javiereguiluz commented Apr 1, 2016

fabpot Apr 3, 2016

Choose a reason for hiding this comment

linaori commented Apr 6, 2016

xabbuh commented Apr 6, 2016

linaori commented Apr 6, 2016

HeahDude Apr 6, 2016

Choose a reason for hiding this comment

linaori Apr 7, 2016

Choose a reason for hiding this comment

wouterj Jun 12, 2016

Choose a reason for hiding this comment

linaori commented Apr 15, 2016 • edited Loading

wouterj commented Jun 11, 2016

linaori commented Jun 12, 2016

wouterj Jun 12, 2016

Choose a reason for hiding this comment

wouterj commented Jun 12, 2016

xabbuh commented Jul 4, 2016

linaori commented Jul 4, 2016

xabbuh Jul 4, 2016

Choose a reason for hiding this comment

xabbuh commented Jul 4, 2016

linaori commented Jul 4, 2016

xabbuh commented Jul 5, 2016

wouterj commented Jul 5, 2016

linaori commented Jul 5, 2016

linaori commented Apr 15, 2016 •

edited

Loading