[HttpKernel] Give higher priority to adding request formats #20871
Conversation
|
It would be nice if this could be added to the next 3.2.n release... |
|
But this could indeed break applications or bundles where an event listener analyses or modifies the request formats. |
|
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? |
|
We could at least mitigate that issue a bit by changing the listener's priority to 1. @symfony/deciders What do you think? |
|
A priority of 1 would indeed work. I used 255 as it is the highest value for the usually 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! |
|
Can someone give some input here? I think it would be nice if we can continue working on this :) |
|
ping @symfony/deciders :) |
|
Looks reasonable to me. |
Lower the AddRequestFormatsListener's kernel request event priority from 255 to 1, to be the least bc incompatible.
|
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... |
|
Can you also update the test? |
|
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? |
|
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. ## Problem It's possible to extend the formats known by the `Request` using a piece of config: ```yaml framework: 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 solution Increasing 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
* 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.