Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Give higher priority to adding request formats#20871
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
akeeman commentedDec 11, 2016
It would be nice if this could be added to the next 3.2.n release... |
xabbuh commentedDec 19, 2016
But this could indeed break applications or bundles where an event listener analyses or modifies the request formats. |
akeeman commentedDec 19, 2016
Indeed. Though this is only the case when these criteria are met:
In my opinion, a bug is exploited if that is the case. It's a weird feature to be able to register custom formats, but to not be able to use them in a stage where it makes perfect sense to do so. Of course Symfony should be stable and changing this could break thinks. And keeping that in mind is important too. Therefore I'm glad@xabbuh expresses his concerns, though I do wonder what your opinion is about merging this PR. Do you think it's still possible to merge it into 3.2.n as I proposed? Or rather wait to 3.3.0, so it's available in May? Or don't you feel like this is a good plan at all? |
xabbuh commentedDec 19, 2016
We could at least mitigate that issue a bit by changing the listener's priority to 1. @symfony/deciders What do you think? |
akeeman commentedDec 19, 2016
A priority of 1 would indeed work. I used 255 as it is the highest value for theusually used range, and, as you noticed, I think it should run quite early as it doesn't have dependencies, can be dependent of it (after all, that's what it's for), and is an optional and quite fast operation. For many people (or at least me) who want to use this feature during the kernel.request event, increasing it to 1 for now would work. Maybe it's an idea to set it to 1 for 3.2.n and to 255 for ^3.3.0? Let me know your thoughts! |
akeeman commentedJan 18, 2017
Can someone give some input here? I think it would be nice if we can continue working on this :) |
xabbuh commentedJan 18, 2017
ping @symfony/deciders :) |
dunglas commentedJan 18, 2017
Looks reasonable to me. |
Lower the AddRequestFormatsListener's kernel request event priority from 255 to 1, to be the least bc incompatible.
akeeman commentedJan 18, 2017
Any chance you guys will accept an additional pr with a priority of 255 instead of 1 targeted milestone 4.0 or something? It's more logical that it has this value after all if bc breaks are accepted... |
xabbuh commentedJan 18, 2017
Can you also update the test? |
fabpot commentedJan 18, 2017
I'm going to merge it in 2.7 as it is a bug fix. We could also change it to 255 in 4.0, but is it really worth it? |
fabpot commentedJan 18, 2017
Thank you@akeeman. |
…s (akeeman)This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes#20871).Discussion----------[HttpKernel] Give higher priority to adding request formats| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | yes| BC breaks? | yes (unlikely to break projects*)| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | not documented\* Only applies to the unlikely case where formats are used before this event is loaded key. Users must have specified custom formats, and actually using them should then break their application. I think that's an unlikely case.## ProblemIt's possible to extend the formats known by the `Request` using a piece of config:```yamlframework: request: formats: json: 'application/merge-patch+json'```Using this in a `kernel.request` event is currently not possible, as it loads after any custom events.## Offered solutionIncreasing the priority of the AddRequestFormatsListener listener resolves this problem.This change only impacts projects that use the `framework.request.formats` configuration key, as the listener is not loaded if it isn't defined.Commits-------9edb457 [HttpKernel] Give higher priority to adding request formats
Uh oh!
There was an error while loading.Please reload this page.
* Only applies to the unlikely case where formats are used before this event is loaded key. Users must have specified custom formats, and actually using them should then break their application. I think that's an unlikely case.
Problem
It's possible to extend the formats known by the
Requestusing a piece of config:Using this in a
kernel.requestevent is currently not possible, as it loads after any custom events.Offered solution
Increasing the priority of the AddRequestFormatsListener listener resolves this problem.
This change only impacts projects that use the
framework.request.formatsconfiguration key, as the listener is not loaded if it isn't defined.