[HttpKernel] Give higher priority to adding request formats #20871

Closed
wants to merge 3 commits into
from

Projects

None yet

6 participants

@akeeman
Contributor
akeeman commented Dec 11, 2016 edited
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:

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.

@akeeman akeeman Give higher priority to adding request formats
76ca3ac
@akeeman akeeman changed the title from Give higher priority to adding request formats to [HttpKernel] Give higher priority to adding request formats Dec 11, 2016
@akeeman
Contributor
akeeman commented Dec 11, 2016

It would be nice if this could be added to the next 3.2.n release...

@xabbuh
Member
xabbuh commented Dec 19, 2016

But this could indeed break applications or bundles where an event listener analyses or modifies the request formats.

@akeeman
Contributor
akeeman commented Dec 19, 2016

Indeed. Though this is only the case when these criteria are met:

  • application configures custom framework.request.formats
  • application has an event listener on kernel.request that processes the format
  • given that event listener, this one may not consider the custom registered formats.

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
Member
xabbuh commented Dec 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
Contributor
akeeman commented Dec 19, 2016

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!

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone Dec 26, 2016
@akeeman
Contributor
akeeman commented Jan 18, 2017

Can someone give some input here? I think it would be nice if we can continue working on this :)

@xabbuh
Member
xabbuh commented Jan 18, 2017
@dunglas
Member
dunglas commented Jan 18, 2017

Looks reasonable to me.

@akeeman akeeman Lower priority from 255 to 1
Lower the AddRequestFormatsListener's kernel request event priority from 255 to 1, to be the least bc incompatible.
6ed0599
@akeeman
Contributor
akeeman commented Jan 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
Member
xabbuh commented Jan 18, 2017

Can you also update the test?

@akeeman akeeman updated test
cbd9e76
@fabpot
Member
fabpot commented Jan 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
Member
fabpot commented Jan 18, 2017

Thank you @akeeman.

@fabpot fabpot added a commit that referenced this pull request Jan 18, 2017
@fabpot fabpot bug #20871 [HttpKernel] Give higher priority to adding request format…
…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
419bcf6
@fabpot fabpot closed this Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment