Make ContentNegotiation a "internal" service, used only by the router, so that core or contrib can implement real negotiation

I believe the code need is:

1) The mime matcher needs to filter and reorder routes, so that the first-matchable one is what will get picked up by UrlMatcher.
2) We need to ensure we always set a _format attribute on a route that's been matched, so downstream can leverage that.

Updated issue summary.

This fell off my radar, but I'm going to try and put it back on.

I'm not sure if it qualifies as an API change per se. It kind of does, but only for systems most modules won't be touching.

OK, I decided to make a new RouteEnhancer rather than using MimetypeMatcher, since the latter has multiple other patches messing with it.

This will also conflict with the HtmlPage issue. I hate rerolling patches. :-(

Ugh, I did add a related issue. The issue queue is barely usable at this point. :-(

Wonderful!!

Let's just add a unit test.

I am going to un-RTBC this, if only because it will conflict with the HtmlPage issue and that is about 400 times harder to reroll. I'd MUCH rather reroll this issue once that's in, and we're actively working on it now. So, please sit tight, this will be back soon (I hope).

Rerolling now that #2068471: Normalize Controller/View-listener behavior with a Page object is in...

Patch is smaller because half of the work was done already in that issue through related refactoring.

Back to RTBC

+++ b/core/lib/Drupal/Core/Routing/Enhancer/RequestFormatEnhancer.php
@@ -12,9 +12,12 @@
+ *
+ * In this case we are modifying the request, not the route data, but it
+ * is the most logical place to set this value.
  */

Why is it the most logical place?

Because there is no other place to do so. It needs to happen after matching (because we don't know what route we have until then) but before the other enhancers fire (because they rely on knowing the mime type to determine the wrapping controller). There is no operation between matching and enhancing other than a high-priority enhancer.

It's not my preferred place to do so, it's just the only one I know of.

OK I can live with it if it's the only place, but the information in #11 should go in a comment.

Let's just do it.

Once again, dawehner saves my day. :-)

Sorry the more I look at this the less right it looks. Relying on listener priority for fundamental stuff like this feels very fragile indeed. I don't have much suggestion on how to fix though, but a new event in between the two maybe? And even with that comment I still don't know understand we're altering the request in a route enhancer.

In irc I expressed similar reservation as @catch - a router enhancer not enhancing routes just does not make much sense. And also if this is the solution going with a priority of 100 doesn't make much sense either. Maybe 254 or 255 as 255 is the theoretical maximum. But even just considering this shows that the solution is wrong in my opinion.

....

Opened a pull request to have a proper event in place: https://github.com/symfony-cmf/Routing/pull/93/files

We discussed this at length at last week's meeting. What we're going to do instead is take the code as written here, but move the new conneg enhancer and the existing _controller-adding enhancers to their own request listeners that happen post-matching. Order control will still be via priority, but it will be a little less weird than using the route enhancer to modify the request.

Key participants in the discussion: dawehner, msonnabaum, Crell, and lsmith.

Following on from #1959574: Remove the deprecated Drupal 7 Ajax API, this does what was described in the previous comment. I did not touch the AjaxEnhancer since there's still an open question of whether or not we'd even keep it, and the Entity enhancer is, I think, not relevant to this discussion.

Since the noted patch isn't in yet, this is rolled on top of it. Look at the for-review version for what I've changed here.

Grumble grumble issue queue...

I started to reroll the first patch from #20, then when I had a question in irc, @dawehner pointed out that the for review one was just the changes needed since #1959574: Remove the deprecated Drupal 7 Ajax API got in. That applies to head, and passes phpunit tests, so reuploading it with a different name so it gets tested.

this is the same as @Crell's 2026431-accept-format-for-review-do-not-test.patch

first interdiff is a fix for the php error. (phpstorm said that would have worked in php 5.5)
does the fact that the phpunit tests didn't fail mean that there is missing test coverage of that?

second interdiff is nits.

actual nit interdiff this time.

@YesCT Thanks for the rerole!

This could be it.

Thanks, folks. Although this will conflict with #2019123: Use the same canonical URI paths as for HTML routes :-( (That's why I'd been holding off on this one.)

Come on, this is such a small patch compared to the other one.

Yes, I didn't want to break the other one, either. :-)

We could also just mark this one as blocking for the other one and so push it to critial :P

Looks like catch has had his paw prints on this one.

I've been following this, and the idea is fine, I think I'm having trouble getting over my intense dislike of the API for event subscribers, but that's not very useful in the context of this particular issue I guess.

One question though:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php
   @@ -0,0 +1,93 @@
   +    $events[KernelEvents::REQUEST][] = array('onRequestDeriveFormat', 31);
   

   31

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php
   @@ -0,0 +1,93 @@
   +    $events[KernelEvents::REQUEST][] = array('onRequestDeriveContentWrapper', 30);
   

   30

3. +++ b/core/lib/Drupal/Core/EventSubscriber/ContentFormControllerSubscriber.php
   @@ -57,14 +58,29 @@ public function __construct(ContainerInterface $container, ControllerResolverInt
   +    $events[KernelEvents::REQUEST][] = array('onRequestDeriveFormWrapper', 29);
   

   29

Is there really no need to leave any space between these?

Those numbers were chosen because 32 is the priority of the routing itself (selected because that's what Symfony fullstack does; why I have no idea) and we wanted those to run as soon as possible afterward, in a specific order. I cannot think of a use case for injecting something between them; certainly another listener could use the same priority if it doesn't care which runs first, as long as it's before another in the list. Access control is currently 30 (AccessSubscriber), and I felt we wanted the connect to happen first.

If we want to rejigger the priorities of various other listeners to "make room" I'm open to that, but IMO that should be a follow up. Once this is in I want to investigate the alternate library from #1505080-52: [META] Content Negotiation for Accept Headers, which is what this centralization allows us to do.

One major reason why these numbers are like that is that 32 is the routing. We want to update the _content and _controller bit as early as possible after routing, so other subscribers get the actual proper information.
One usecase you might think of could be to derive a different request format. I kinda think we should have an interface for the content negotiation so people could just switch out that service in contrib.

Btw. there is nearly infinite space between two ints as nothing blocks you from using floats.

28: content_negotation-2026431-28.patch queued for re-testing.

[edit: sorry! forgot about #1952058: [META] Retesting stale RTBC core patches manual retests like this wont be needed ]

Alright I guess the floats are there if you really need it, it's not wonderful but it's not completely locking out.

Like the interface idea - could we open a follow-up for that?

Committed/pushed to 8.x, thanks!

The next step IMO is to try swapping our crappy library out for a real one: #1505080-52: [META] Content Negotiation for Accept Headers