-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.2] ParamConverters in FrameworkBundle #1547
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
Comments
@schmittjoh Say you use JMSSecurityExtraBundle. This requires having 2 methods when you don't have a ParamConverter: the action getting the id, and another action called after converting the param as JMSSecurityExtraBundle needs to have the object. |
@schmittjoh I dont use annotations at all, if i use parameter converters for "viewAction(Object $obj)" then i could directly check if all grants are valid and throw a 403 exception otherwise. This would be reusable across all controllers that trigger this parameter converter. Edit: Not sure if this is the right place though, i would need to try it in a real world app to see if there are downsides. |
@beberlei if i read this correctly is the exact same way the work currently, you dont need the annotation i will also take every typehint into considoration. I have this for injecting the current user into my controller actions and other nifty things :) Also if this is moved back (again) the request injection magic should be moved to a param converter. |
I have some questions about the implementation of FrameworkExtraBundle: Why doesn't it allow to override a parameter, i.e. transform a request variable "{from}" into a \DateTime $from. The ParamConverterManager disalllows this. Is there any reason for this? Edit: I think the ParamConverterManager is sufficently protecting overwriting variables multiple times or in the wrong semantics. |
How does the ParamConverterManager disallow this? the only thing you need is to Typecast the $from variable in you action definition. <?php
// ...
public function myAction(\DateTime $from) {
// ... some more stuff
} Then if there is a ParamConverter that matches then it would be called. |
See ParamConverterManager if ($request->attributes->has()) condition which calls unset($configuration[$key]); |
Dosent it only continue because the highest supported converter have converted the request variable? cant see a unset in the code. |
@fabpot any thoughts about it ? |
This is too late for 2.1, i don't mache too much time the next months sadly, so I can't bring this forward. Plus I have some ideas about Param Converters that should probably be tested in Sensio Extra first. |
OK, let's re-schedule the discussion for 2.2 |
We could make it a Component maybe? |
@henrikbjorn maybe, but it is too late to have it for 2.1 IMO (especially if we want to improve the design of the param converters as the current implementation has some limitations) |
What are the current limitations? Especially if we drop Annotation support i cant really see any. |
@henrikbjorn the fact that the current converter does not support being used for several params of the route (unless a fix has finally been merged for this) |
@stof that was fixed some time ago. It dosent support multiple annotations but the behavior described by Benjamin is supported. See https://github.com/sensio/SensioFrameworkExtraBundle/blob/master/EventListener/ParamConverterListener.php#L64 |
I'd be interested in implementing this in HttpKernel without annotation support (IMO that should remain for the FrameworkExtraBundle). Main reason is, that I frequently use components standalone and param converter would be something handy to have. Would this be generally accepted? (if the code is up to par of course) |
This issue should probably be closed because the feature requested by @beberlei is already included in Symfony. |
@javiereguiluz to be exact, the feature is in FrameworkExtraBundle, not in Symfony. @beberlei was asking to move it in core |
@stof I'm glad to read this, because that means that solving this issue will be truly easy. We just need the Symfony core to make a yes/no decision. |
@javiereguiluz I'm in favor of doing this move, but it's not as easy as it sounds. |
I expressed myself really bad. What I though it should be easy is to take a decision, not solving the issue. And reading you comment, the decision is clearly: yes. |
Please note that I've started working on this issue: #11457 Sadly, it can't be done without BC breaks. |
Commits ------- 22c69c0 Cleanup 76c521f Change apply method to always return a boolean value. This makes converting attributes with the name name as a action parameter possible Discussion ---------- Change apply method to always return a boolean value. This makes converti Change apply method to always return a boolean value. This makes converting attributes with the name name as a action parameter possible. symfony/symfony#1547 (comment) Before this patch it was not possible to have a /blog/archive/{date} route and a action like `myAction(DateTime $date)` as the request attribute would already exists and the paramconverter would not be called. This path is breaking BC for the apply method on ParamConverterInterface which now MUST return a boolean value indicating if a conversion was done. --------------------------------------------------------------------------- by fabpot at 2011/08/23 02:43:57 -0700 The semantic of the converters is also changed with this patch. Currently, we only convert parameters that are not yet in the attributes. After the patch, you convert all parameters even if they are already in the request attributes. --------------------------------------------------------------------------- by henrikbjorn at 2011/08/23 03:50:06 -0700 The problem with having to abort the parameter conversion when the attribute is already there is if you give a route a default it will already be present and not converted. --------------------------------------------------------------------------- by henrikbjorn at 2011/08/23 03:50:59 -0700 And the conversion of a parameter is aborted if a converter returns true, the converter should be able to handle default parameters :) --------------------------------------------------------------------------- by henrikbjorn at 2011/08/29 06:44:03 -0700 As you might want to load a default category object if no specific category was provided. --------------------------------------------------------------------------- by henrikbjorn at 2011/11/25 08:15:11 -0800 @fabpot rebased and tests pass :) good to go.
@wouterj didnt see you already started working on this tried to extract it from the FrameworkExtraBundle https://github.com/henrikbjorn/ParamConverter but i am all for refactoring and rethinking the approach. |
What state is this in? It looks like there was some voting proposal but just initiator voted. @henrikbjorn Good job for extracting the feature! |
…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
We should look at ways to move param converters to the framework bundle again. Instead of defining the converters on a per variable basis as in extra bundle @ParamConverter the converters should be registered globally and be matched on the hinted class, ordered by priority of the converters registered:
Say:
Then FrameworkBundle would look if it has one or many parameter converters registered for "Post".
If it has registered converters then based on a priority each parameter converter tries to convert the variable based on the data in the current request. It returns true if it could convert it, false if it couldnt. If no converting mechanism could be found for a class through all the current request data then it should throw an exception.
This would allow to remove tons of glue code with 404 and security handling and such from the controllers, into the param converters making it much more reusable accross different controllers.
The text was updated successfully, but these errors were encountered: