Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Documented the ArgumentResolver along the ControllerResolver#6422
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
00e0e7b to678c705Comparebd2fb26 to947626cComparejaviereguiluz commentedApr 1, 2016
@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 commentedApr 1, 2016
@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 commentedApr 1, 2016
@iltar you are doing great! It's just a matter of minor "cosmetic changes". But for now don't worry about them 😄 |
linaori commentedApr 1, 2016
@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 commentedApr 1, 2016
@iltar we still use |
25f4985 to2eecbd2Compare66ee94b to1ce0cc7Compare| ..caution:: | ||
| The `getArguments()` method in the:class:`Symfony\\Component\\Httpkernel\\Controller\\ControllerResolver` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
getArguments must be surrounded by two `
…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 ArgumentResolverf29bf4c Refactor ArgumentResolverTestcee5106 cs fixescfcf764 Added an ArgumentResolver with clean extension point360fc5f Extracting arg resolving from ControllerResolver
linaori commentedApr 6, 2016
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 commentedApr 6, 2016
linaori commentedApr 6, 2016
@xabbuh it doesn't really matter, one describes the feature, the other makes sure the documentation doesn't trigger deprecations in your code. |
| :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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing colon ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Moved the caution to below, this also fixes the issue where the : refers to a caution instead of the intended code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then it should be a full stop (.).
24b11e2 toa9897c7CompareThis 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 commentedApr 15, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Broken build is related to: :doc:`/cookbook/controller/argument_value_resolver` This will be solved as soon as#6438 is merged. |
wouterj commentedJun 11, 2016
Merged#6438 now. Can you please rebase? |
a9897c7 to4edfe5aComparelinaori commentedJun 12, 2016
@wouterj rebase is done and checks pass now |
| :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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
remove the comma beforeand
wouterj commentedJun 12, 2016
Can you please also fix the outstanding comments (both from me and others)? Sorry for not reviewing it before asking you to rebase. |
xabbuh commentedJul 4, 2016
@iltar Is this still a work in progress from your point of view? |
linaori commentedJul 4, 2016
@xabbuh I totally forgot to fix the title! No, this should be finished. |
| $response->send(); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be removed
xabbuh commentedJul 4, 2016
👍 |
a26f622 to3eda91dCompare3eda91d tof630d8dComparelinaori commentedJul 4, 2016
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 commentedJul 5, 2016
👍 still looks good to me Status: Reviewed Note to merger: This needs to be merged in the |
…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 commentedJul 5, 2016
linaori commentedJul 5, 2016
@wouterj I've checked it, LGTM |
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.