Implement StackNegotiation

Crell asked to leave this parked

Actually this one is fine. It's the old meta I meant to leave parked. :-) Working on this now.

Here we are. Let's see how well this works.

This includes a new composer library so please look at the do-not-review patch.

And of course I find one bug right after I post those...

I really need to stop writing code after midnight. It's like a gremlin. Nothing good comes of it.

OK, this just installed for me locally. Why is it complaining about missing files?

I don't see nothing problematic, beside that fact that you forgot to add the actual code.

1. +++ /dev/null
   @@ -1,43 +0,0 @@
   - */
   -class AjaxSubscriber implements EventSubscriberInterface {
   -
   

   This is really cool!

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php
   @@ -84,8 +52,7 @@ public function onRequestDeriveContentWrapper(GetResponseEvent $event) {
   +    $events[KernelEvents::REQUEST][] = ['onRequestDeriveContentWrapper', 30];
   

   this is so much more readable...

3. +++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.php
   @@ -88,7 +76,7 @@ public function onView(GetResponseForControllerResultEvent $event) {
   -      $method = 'on' . $this->negotiation->getContentType($request);
   +      $method = 'on' . $request->getRequestFormat();
   

   This indeed also improves the DX

With dawehner's help I managed to figure it out. I had referenced a dev version of the library so it installed with a .git directory in it, despite our composer.json file saying to prefer dist. That of course screwed up git entirely. I checked and the current 1.0-dev is identical to the 0.1.0 tag, so I switched to that instead and it should be better now.

No interdiff because I had to do some fierce rebasing to sort it all out, which makes it hard to recreate an interdiff. The only change is the version number specified for the stack-negotiation library, though.

Ha! language module was a bad boy and didn't prefix a service name. Oh well.

This should take care of caching. I'm not sure what's going on with the REST tests...

@crell
Did you really considered this to be easy?

• You thought IE is crazy, you haven't see the Accept header it sends, see https://github.com/willdurand/Negotiation/issues/34
• Still, if the negotation could not return anything we should still fail. We need to solve the issue in the negotiation library, we can't really workaround on the routing level itself.
• Let's ensure to register hal+json on the negotiator, but yeah maybe the external library could do that for us

+++ a/core/modules/rest/src/Tests/ReadTest.php
@@ -54,7 +54,8 @@ public function testRead() {
+      // @todo We send an invalid accept header, we should return 406.
+      $this->assertResponse(404);

406 or 404, pick one. :-)

Also as discussed in IRC today: To handle broken browsers, we'll add another middleware that fires after the Negotiation one. All it does is check for a missing _format and set it to HTML, on the assumption that only a busted browser would send an unmanageable Accept header and busted browsers probably want HTML anyway. (That's roughly what the assumption was in D7, too.)

• Decided to wrap the format negotiator as there are example of which browsers asking for application/xml even, they actually want HTML, so the original negotiator "did worked", so let's ensure that this doesn't happen
• This now also throws an exception in case no type was found

Can we have a do-not-test without the library itself?

+++ b/core/lib/Drupal/Core/StackMiddleware/BrowserBugsFormatNegotiator.php
@@ -0,0 +1,101 @@
+
+    return $this->formatNegotiator->getBest($header, $priorities);
+  }

I believe this should be coming _before_ checking for custom headers so that it is a true fallback.

Also:

Reviewing the negotiation code I see that all our fallback browsers have */*, so should it not be simpler to add a list of priorities?

e.g. array('html', 'json', 'hal')

or calling getBest() again with some hardcoded priorities?

Because in case of a wildcard match */* the negotiator would by default use the first priority format that is not */*.

As far as I can see the negotiator should still match if a browser explicitly asks for application/json, etc.

But that would need testing.

I am not opposed to the decorator - though I think the upstream interface is misleading as it has too less functions compared to what FormatNegotiator actually implements as public. We should fix that upstream.

+++ b/core/lib/Drupal/Core/StackMiddleware/BrowserBugsFormatNegotiator.php
@@ -0,0 +1,101 @@
+    // In case negotiation couldn't figure out the format, try to check for
+    // browser fallbacks.
+    $browser_accept_headers = [

I just re-read the comment for the with-library patch and I think we can disregard my previous comment.

Can we just change this comment then to not speak of a fallback, but as a special case or such?

And fix the upstream interface to match?

@Fabianx
You are right the comment is wrong and is coming from an earlier implementation.

We do have an issue upstream already https://github.com/willdurand/Negotiation/issues/34 but yeah discussions are a bit difficult.

I just re-read the comment for the with-library patch and I think we can disregard my previous comment.

Do you still have some questions? Feel free to ask.

Adapted the comment.

> We do have an issue upstream already https://github.com/willdurand/Negotiation/issues/34 but yeah discussions are a bit difficult.

My main concern was that the BrowserBugsFormatNegotiator implements the FormatNegotiatorInterface, but that has just 4 functions, while BrowserBugsFormatNegotiator and FormatNegotiator have 8 public functions.

So the interface is lacking upstream, which could be a problem in the future if something changes in the FormatNegotiator class, where we don't have a contract.

Alternatively we could just implement the contracted interface and add magic __call method to pass through all other function calls to the class (true wildcard decorator).

The patch here is fine though :).

Lets discuss what we can do about the interface and then this is RTBC :).

@Fabianx
I see what you mean, thought you have to take into account that the Negotiation stack class takes FormatNegotiatorInterface $formatNegotiator = null
so we should implement that interface. But you are right, we should call all methods on the decorator class, you never know.

RTBC - Nice work! - Maybe add another do-not-test.patch so its easier for reviewers to see the relevant changes?

Sure.

+++ b/core/lib/Drupal/Core/StackMiddleware/BrowserBugsFormatNegotiator.php
@@ -0,0 +1,107 @@
+ * @file
+ * Contains \Drupal\Core\StackMiddleware\FormatNegotiator.
+ */

nit - Should this be BrowserBugsFormatNegotiator?

Sure.

nit - + * Contains \Drupal\Core\StackMiddleware\BrowserBugsFormatNegotiato.

Lets add the r :).

Meh^2

Note to committers: While I'm super +1 on this, it is going to conflict with #2323759: Modularize kernel exception handling. Since that one removes code that this patch modifies (and thus would not need to modify anymore) I think it's better to commit that one first, then this one after a (hopefully easy) reroll.

#2323759: Modularize kernel exception handling has landed

Hopefully a good reroll. remove a couple of old referencs and the ContentNegotation class itself.

• Hal should now implement its own exception handling, fixed that
• HalSubscriber is pretty much pointless now
• RequestHandler uses $request->getMimeType() even it should better use the result from the negotation

1. +++ b/core/modules/hal/src/EventSubscriber/HalJsonExceptionSubscriber.php
   @@ -7,7 +7,72 @@
   +      'Content-type' => 'application/hal_json'
   

   Incorrect mime type. It's application/hal+json

2. +++ b/core/modules/hal/src/EventSubscriber/HalJsonExceptionSubscriber.php
   @@ -7,7 +7,72 @@
   +    if ($exception instanceof HttpExceptionInterface) {
   

   This is always true if you're using the base class properly.

3. +++ b/core/modules/hal/src/EventSubscriber/HalJsonExceptionSubscriber.php
   @@ -7,7 +7,72 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getSubscribedEvents() {
   +    $events[KernelEvents::EXCEPTION][] = ['onException', -250];
   +    return $events;
   +  }
   

   Use the base class's abstract method, Luke!

   And then onHalJson() should use the on403, etc. method names. If you don't want to do that, don't use the base subscriber class. It's either/or.

4. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -98,7 +98,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   -      $headers = $e->getHeaders() + array('Content-Type' => $request->getMimeType($format));
   +      $headers = $e->getHeaders() + array('Content-Type' => $request->get('_mime_type'));
   

   Doesn't getMimeType() already honor the result of the negotiation process...?

And then onHalJson() should use the on403, etc. method names. If you don't want to do that, don't use the base subscriber class. It's either/or.

This was actually was I intended to use.

Doesn't getMimeType() already honor the result of the negotiation process...?

Yeah, it is a shame, you could argue that Request::setFormat() should be called from inside the negotiation.

class Request {
    public function getMimeType($format)
    {
        if (null === static::$formats) {
            static::initializeFormats();
        }

        return isset(static::$formats[$format]) ? static::$formats[$format][0] : null;
    }

So @Crell said on #2135829-12: EntityResource: translations support that this would make us remove the HTTP language negotiator specific to Drupal. I don't see that being done on the patch so I guess that was planned to be a followup? I am wondering if Drupal 8's extent of working with the Accept-Language header was taken into account when thinking about this. One of the improvements of Drupal 8 over Drupal 7 is now Drupal 8 understands that the language codes Drupal is using may or may not be what language codes an arbitrary browser would send back as a language the client wants. So Drupal 8 has this UI configurable mapping system where we ship with several default mappings and you can add more as needed. This was especially pushed forward by some Chinese users where the language codes are really various: http://cgit.drupalcode.org/drupal/tree/core/modules/language/config/inst...

So long as this system cannot reproduce that, it can stay along as yet one more system doing language negotiation, but it will not be able to replace the Drupal 8 language negotiation system.

BTW for background on the language mapping system in our negotiation, see #365615: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings).

Stumbled upon some fun issue during day to day work, thank you @webflo :(

@Gábor Hojtsy
No, this issue doesn't deal directly with it, though it adds support that you can fetch it, if you like. Do you think the issue above should leverage the information added by this issue?

Okay, let's keep the hal subscriber for now.

@dawehner: I think the language negotiation part of this 3rd party lib / patch will be useful once it passes all our existing language negotiation tests, which are results of lots of battle testing, and got huge improvements in Drupal 8 even compared to Drupal 7. (Including the configurable language mappings). Until then, it is duplicate functionality that we'll not use. I sense there may be lots of upstream contribution coming out of getting our learnings up to the 3rd party lib. I will unfortunately not be able to devote time to that, its not personal just too much on my plate :/

@Gábor Hojtsy
Just to be clear, this is just about language negotiation for REST requests where you kind of expect to be able to use the headers.
Obviously, this method doesn't work on any normal HTML request.

Noone forces you to put work on it, but at least for the REST issue, this is the way to do it.

@dawehner: are REST clients to be believed to be more consistent than browsers and not use eg. various language codes for Chinese, and instead exactly the codes that Drupal happened to pick a handful of years ago? In other words, is it ok to expect REST clients to need to use the exact internal language code representations of Drupal rather than some other totally valid language code according to the internet standards?

dawehner: Can we have a -do-not-test for review?

Gabor: To the point of this issue, this will have a side effect of the _accept_language request attribute being set based on a reliable HTTP-only negotiation algorithm. Whether that string is useful or not to you is a task for a separate issue, as it doesn't do any validation on the language string itself as far as I'm aware. Use it, don't, that's irrelevant to this thread which is more interested in the Accept header than Language header.

@Crell: two days ago on #2135829-12: EntityResource: translations support you wrote

#2331919: Implement StackNegotiation makes it trivial to have the HTTP language negotiated and marked on the Request object before HttpKernel is even run. In fact I think the current patch would already do that. We can then remove our own HTTP language negotiation if we have any, or make it just wrap that value.

This was the reason I came here. I wanted to point out this does not let us to remove our own HTTP language negotiation or make it wrap the value that this patch produces at least in the current state of the library included. Since in the other thread you said this value will be useful, I thought its worth pointing out that it will not be. If it is not even the goal of this issue to have a useful accept language value for now, that is totally fine, we'll just not use that value then. Unfortunately the goal is/was not clear from the summary, so I went after what you indicated in your comment on that other issue.

I thought it would be. I wasn't aware that there's a dozen non-standard ways to define "Chinese". :-) (And I was trying to explain dawehner's comment in that thread; I think that whole line was a tangent. Sorry for the distraction.)

Sure.

Let's do this!

Back to RTBC then, we can always improve the negotiation handling further for i18n.

Rerolled, single bit in AccessHeaderMatcher.php

There we go.

One more time...

Reroll

This seems impressively fragile, so...

This is a new lib so AFAIK is Dries's.

Off-hand though:

+    "willdurand/stack-negotiation": "0.1.0"

Um. :) We are planning to ship a product used by governments, companies, etc. all over the world for hugely important, high-traffic sites. Any idea what this means, and what the path to 1.0.0 is? :)

Also this would be easier to commit if the issue summary contained information on what this buys us, and what the consequence of not committing it is.

As with Drupal modules, something being not-1.0 means pretty much nothing. There are Drupal modules used by over 100,000 sites that have never had a non-beta release. 1.0 of that lib has only a dev release at the moment, which I wanted to avoid (in part because composer pulled in a git repo for it). I didn't see any meaningful differences between the tag and -dev version before.

Tweaked issue summary, too.

This looks like a very good improvement. Having a properly setup Request object with an already negotiated content format simplifies a lot of code dealing with that. Some minor problems:

1. +++ b/core/lib/Drupal/Core/StackMiddleware/BrowserBugsFormatNegotiator.php
   @@ -0,0 +1,107 @@
   +class BrowserBugsFormatNegotiator implements FormatNegotiatorInterface {
   

   This needs some more docs. Why do we need this? What is this?

2. +++ b/core/lib/Drupal/Core/StackMiddleware/BrowserBugsFormatNegotiator.php
   @@ -0,0 +1,107 @@
   +    return call_user_func_array([$this, $name], $arguments);
   

   So you want to hijack all method calls? Please add a comment. And I think you mean $this->formatNegotiator here? Do we actually need this?

3. +++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
   @@ -77,6 +85,8 @@ public function testAcceptFiltering($accept_header, $included_route, $excluded_r
        $request = Request::create('path/two', 'GET');
        $request->headers->set('Accept', $accept_header);
   +    $format = $this->negotiator->getBest($accept_header = $request->headers->get('Accept'));
   +    $request->setRequestFormat($request->getFormat($format->getValue()));
   

   Why do we even have to invoke the content negotiation here? Just set the format on the request and be done with it for the exercise of this test? It should not also test the content negotiation, that would be a different unit test.

4. +++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
   @@ -77,6 +85,8 @@ public function testAcceptFiltering($accept_header, $included_route, $excluded_r
   +    $format = $this->negotiator->getBest($accept_header = $request->headers->get('Accept'));
   

   The variable $accept_header is never used afterwards?

5. +++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
   @@ -103,6 +113,8 @@ public function testNoRouteFound() {
   +    $request->setRequestFormat($request->getFormat($format->getValue()));$this->matcher->filter($routes, $request);
   

   2 statements ending with ";" on one line?

This needs some more docs. Why do we need this? What is this?

Improved the documentation, feel free to give some feedback on it.

So you want to hijack all method calls? Please add a comment. And I think you mean $this->formatNegotiator here? Do we actually need this?

Yeah this was pretty much pointless, as yeah, even the implementation was broken, tbh.

Why do we even have to invoke the content negotiation here? Just set the format on the request and be done with it for the exercise of this test? It should not also test the content negotiation, that would be a different unit test.

Good point.

2 statements ending with ";" on one line?

Yeah this happened on accident.

Let's reapply.

This patch seems to make things more maintainable and fixes a bug.

Based on the policy outlined in #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?, this issue was worked on during DrupalCon Amsterdam, so if it is ready by Oct. 28, we will consider it for inclusion.

Otherwise, we need to evaluate whether this issue is in fact fixing a major bug or other important stabilizations listed in that policy, we will need to evaluate the impact versus the disruption. So, let's add clarification to the issue summary regarding what specific bugs this may actually be solving, including test coverage for them, and reference any related issues.

Per @effulgentsia, a related bug was raised in #1505080-40: [META] Content Negotiation for Accept Headers. Should we file an explicit bug report for that?

Does this also provide a way to resolve #2339491: Ajax requests on forms with files fail on IE 9?

@Dries, @xjm

There are several points:

• We should have proper content negotitation, regardless of bugs, in order to support REST. Not having that would be headless, so let's add it to be headless (more bullshit words insert here :P).
• We already have good test coverage for our crazy edge cases like for our best friend IE, so we are sure we don't add a regression here.
• @xjm This does not solve #2339491: Ajax requests on forms with files fail on IE 9 to be honest, because that is a response, while content negotation is more about figuring stuff out for the response.
  After this issue I'll probably do some research on that one.
• Should we file an explicit bug report for that?If someone wants to invest the time and aggregate the information from issues like #2026431: Make ContentNegotiation a "internal" service, used only by the router, so that core or contrib can implement real negotiation have fun, but I just think
  that there are better ways to invest our time.

Another reroll, note: This patch was once RTBC a month ago already.

Let's drop that silly tag if it doesn't help anything anyway :)

As already noted in the issue summary the API impact here is extremely small. It's very unlikely that any existing modules would be impacted, and even if they are the change is trivial. (You now can always rely on $request->getRequestFormat(), whereas before you only sometimes could so had to use our crappy old Conneg class manually.)

This has been RTBC before I think it's safe to re-RTBC with tests passing.

Patch is still RTBC

here the reroll.

Once more...

Fascinating: the word performance does not even appear in this issue. Although usually Drupal 8 doesn't care much about performance hoping that caching will take care of everything, this is way too early for that.

@chx
I really hope we use the same requirements for all issues around ... working on a benchmark now.

Run without page caching

Run 5446d0de206d8 uploaded successfully for drupal-perf-dawehner.
Run 5446d42931309 uploaded successfully for drupal-perf-dawehner.
<code>
=== 8.0.x..2331919-stackneg compared (5446d0de206d8..5446d42931309):

ct  : 0|0|0|N/A%
wt  : 412674|414851|2177|0.5%
cpu : 408078|410103|2025|0.5%
mu  : 21875352|22011816|136464|0.6%
pmu : 21954544|22090240|135696|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5446d0de206d8&...

Page caching enabled

Run 5446d71499e7c uploaded successfully for drupal-perf-dawehner.
Run 5446d73a3daba uploaded successfully for drupal-perf-dawehner.

=== 8.0.x..2331919-stackneg compared (5446d71499e7c..5446d73a3daba):

ct  : 0|0|0|N/A%
wt  : 9428421|9828964|400543|4.2%
cpu : 8922289|9321539|399250|4.5%
mu  : 46415792|48035016|1619224|3.5%
pmu : 46824376|48443216|1618840|3.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5446d71499e7c&...

Something looks strange about this benchmarks. missing ct is 'interesting'. Maybe a uprofiler incompatibility?

If the profiling numbers in #92 are accurate, then there's the question of whether what this offers us is worth that cost. I looked at what the library does and how the patch integrates it, and here's my 2 cents:

Our current content negotiation is very primitive, and does not handle weighting in the Accept header at all.

From what I can tell, this is true. Symfony's Request::getAcceptableContentTypes() returns mime types in the order they appear in the header, not in quality order. And our ContentNegotiation::getContentType() just uses that order: it doesn't implement a sort by quality either. So, yeah, the StackNegotiation library is nice in that it implements http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html, but in practice, I don't know if there are user agents that we care about that don't order their accept header in order of preference to begin with, so it's possible the benefit is more theoretical than practical.

We re-negotiate every time we need the format rather than leveraging the property on the Request object that is designed specifically to hold the result of negotiation. That means $request->getRequestFormat() works sometimes, but not all the time.

Yes, but that's because our ContentControllerSubscriber::onRequestDeriveFormat() runs at a fairly low priority. We can raise its priority, or even implement it as a Stack middleware without incorporating the StackNegotiation library.

Per @effulgentsia, a related bug was raised in #1505080-40: [META] Content Negotiation for Accept Headers. Should we file an explicit bug report for that?

That bug now appears fixed, regardless of this patch, by virtue of HEAD's AcceptHeaderMatcher. What's left related to that is that RestExport::initDisplay() has this code:

$request_content_type = $this->contentNegotiation->getContentType($this->view->getRequest());
// Only use the requested content type if it's not 'html'. If it is then
// default to 'json' to aid debugging.
// @todo Remove the need for this when we have better content negotiation.
if ($request_content_type != 'html') {
  $this->setContentType($request_content_type);
}

Given that AcceptHeaderMatcher ensures that this code only runs if the agent accepts what the route can provide, the above code is not incorrect. But it's unclean. It would be nice if $request_content_type wasn't set to 'html' to begin with if the route doesn't return HTML. But the patch in this issue doesn't fix that. It potentially lays the groundwork for that fix, since the library's getBest() method includes a $priorities parameter, so after routing, we could invoke it to find out the best content type that the browser accepts that the route can return. But potentially, we can do something similar even without the better library.

In summary, I think the library is cool, and being RFC compliant is cool, but if we think the library is too slow and are willing to sacrifice RFC compliance in favor of performance, then I think we can still implement cleanups to code like the above even without the library.

Alex: Daniel and I discussed the possibility of doing a "current lib refactor" patch in the WSCCI meeting this morning; same as we're doing here, but with a middleware for the current crappy conneg lib rather than StackNegotiation, so at least getRequestFormat() becomes reliable. It should also have no performance impact as it's just moving existing code around. (If anything there might be a negligible improvement from not calling the lib multiple times.)

I still think we should do proper negotiation, but at least that way switching to it later would be zero API change instead of minimal edge-case change if we're still going to be stuck on performance measurements here.

> It should also have no performance impact as it's just moving existing code around

Do we negotiate currently if the page cache fires?

Edit: I seriously doubt. But I guess there's some ordering of middlewares we can exercise...?

This deserves a separate comment: page cache currently is a 200 priority middleware. For some reason this patch adds negotiation with a 400 priority. Can this patch be salvaged by changing the priority to 100?

Yeah I would not really trust those benchmarks (probably too much actual load on my local machine, which seems just matter on page caching as there is less work to do in general),
but at least the absolute number of function calls is deterministic.

ct  : 1233010|1233165|155|0.0%

After a run debugging / discussion session with @fabianx, @larowlan, @chx we realized that the content-neg is required to run properly
for page caching, as #1855260-44: Page caching broken by accept header-based routing already figured out.

If you have application/hal+json as accept header, you will retrive the HTML cache entries, because the hal subscriber, comes after page cache.

This patch fixes the bug already.

I believe that #2019123: Use the same canonical URI paths as for HTML routes was an utterly bad idea. Not only the internal page cache is broken, but also every proxy cache. Unless perhaps if we start to add Vary: Accept for routes which are also accessible via REST. Catch noted in #1855260-81: Page caching broken by accept header-based routing (and I very much agree):

This still feels incredibly fragile overall

It seems that page caching was not discussed at all in #2019123 and I think we might want to reconsider this. Also note REST - Tradeoffs between content negotiation via Accept header versus extensions on SE.

Vary: Accept sounds great to me and the router knows if there is more than one route on the same path, so should be relatively "simple".

The REST thing is important to be REST-compliant.

The REST thing is important to be REST-compliant.

REST does not mandate that the same paths are used as for the HTML content. I very much expect that existing implementations in other serious projects do not use the same paths for HTML and REST because of the implications for caching etc.

Akamai doesn't work with Vary: Accept see #1855260-25: Page caching broken by accept header-based routing.

Also chx linked to https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation and http://jamesrdf.blogspot.ca/2009/09/accept-headers-in-wild.html in irc.

Relying on browsers to send exactly the same accept header all the time is not going to result in good hit rates.

Thanks znerol for finding the two year old issue where this was already pointed out in depth, before #2019123: Use the same canonical URI paths as for HTML routes was even opened.

I take back what I said about Accept: Vary and REST. I got more information now.

I still think that its better to leave things now for this issue and open / re-open another one for that kind of things.

Maybe we should wait with this patch until #2230121: Remove exit() from FormBuilder get solved. Because the have some difficulties with the issue and moshe weitzman has said (comment #39):

I might get tomatoes thrown at me, but if the Critical problem here is breaking StackPHP, then we could consider rolling back StackPHP. Having forms POST to dedicated routes is a huge change at this point in the release cycle.

Maybe we should wait with this patch until #2230121: Remove exit() from FormBuilder get solved. Because the have some difficulties with the issue and moshe weitzman has said (comment #39):

Note: Once we have this code in place we could also just replace the stack implementation with an event subscriber which happens way before routing and be done.

#106 I don't see how an event is better here than the stack you can "subscribe" to ...

/me confused

What URLs something lives at is off topic for this thread. However, it's also not a binary "REST vs. HTML" question. We support, out of the box, at least 3 if not four different formats on entities. (HTML, JSON HAL, raw JSON, and I think XML HAL but I forget if we implemented that one.) Putting all of them at different URIs is not reasonable, so just splitting HTML and "not HTML" off to different URIs won't solve anything cache-related.

If anything, doing proper negotiation pre-cache lets us use the resolved accept format as a cache key rather than the full Accept header. That doesn't help with Varnish, true, but as above just splitting the URIs for HTML and REST wouldn't help with that either.

The question here is:
1) Is this architecturally better than what we have now. (IMO, unequivocally yes because now getRequestFormat() works reliably and we can clean up a lot of other code.)
2) Is using a more robust conneg library like StackNegotiation worth its performance impact (depends on what that impact is, which it sounds above like there's some question about).

The use of wrapped kernels itself is a red herring here.

Filed #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use.

This needs profiling, besides this, this is RTBC.

Per Dries this should be ready till tomorrow Oct 28th Amsterdam deadline.

So lets get this in ASAP.

Result with Drupal 8 standard installed:

=== baseline-8.0.x..8.0.x compared (544f2052f26a3..544f21907c73a):

ct  : 62,627|62,627|0|0.0%
wt  : 133,556|133,695|139|0.1%
mu  : 17,968,824|17,968,824|0|0.0%
pmu : 18,055,304|18,055,304|0|0.0%

=== baseline-8.0.x..stack-neg compared (544f2052f26a3..544f21b2ca3f4):

ct  : 62,627|62,845|218|0.3%
wt  : 133,556|134,610|1,054|0.8%
mu  : 17,968,824|18,073,192|104,368|0.6%
pmu : 18,055,304|18,158,824|103,520|0.6%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

PHP 5.5 with opcache, front page, no content, just stock installation

TL;DR:

0.8% overhead, around 1 ms, 218 more function calls, 100 kB more memory usage, measured several times, margin of error is around 0.1 ms

Looks reasonable to me.

EDIT (this was done after Crell already re-RTBCed), results for page cache:

=== baseline-page-cache-8.0.x..8.0.x compared (544f2dea01667..544f2e7153739):

ct  : 3,604|3,604|0|0.0%
wt  : 10,022|9,962|-60|-0.6%
mu  : 2,579,008|2,579,008|0|0.0%
pmu : 2,589,368|2,589,368|0|0.0%

=== baseline-page-cache-8.0.x..stack-neg compared (544f2dea01667..544f2e8d5019b):

ct  : 3,604|3,859|255|7.1%
wt  : 10,022|10,530|508|5.1%
mu  : 2,579,008|2,668,040|89,032|3.5%
pmu : 2,589,368|2,677,728|88,360|3.4%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

Again around 1ms (here even only 0.5 ms). Where 5% sound much, but its really just the difference between 10ms and 10.5 ms, which is perfectly okay ...

XHProf-Kit had a bug, that is why dawehener got the MAX instead of the minimum result ...

On that topic:

10.5 ms for a full page cache retrieval? Wow, that is really fast.

I think a 1 ms overhead for the cleanup here plus considerably more robust conneg is an acceptable cost. Thanks, Fabian!

Back to RTBC.

I 100% agree with RTBC, it really is below 1 ms of overhead - I also tried different Accept formats.

The Format Negotiation library getBest() call itself showed as an overhead of 0.258 ms, which should be totally okay for the win.

I have no opinion about the RTBC (really) but I really would not like to see another page cache affecting issue RTBC'd without any profiling or even considering the performance hit. I am absolutely shocked to see who everyone participated in this issue (even Dries!) and noone asked about performance. I think I will keep my avatar.

In #81, Dries did not raise objections to the library itself. Since this now comes down to a performance cost acceptability question, assigning to catch.

This issue really is a fantastic textbook example of everything wrong with Drupal 8. Not just that three core committers, two initiative leads and more subsystem maintainers than I want to count have skipped raising the performance question (even after webchick pointed out that Drupal is used on high traffic sites!!) but also there is only one comment here which "addresses" the version question and that is #73 which is eerily reminiscent of the handwaving in #1874530-2: Add Symfony CMF Routing component -- basically the answer is, version numbers are meaningless and... and that's it. I could list questions all day long: who did the review of the test coverage of this library ? Issue summary says "it's well-tested" -- well, thanks. Any coverage numbers? If it's as used as it is claimed, well, any blog posts, case studies etc -- there's absolutely nothing of the sorts. And altogether, why this particular library? Where is the research done to compare the available solutions?

This shouldn't stop the commit of this one because it's not worse than so many others we added just this happened to completely blew my fuse.

RE: research ... updated the IS. Quite some research was done.

I do agree it was pre-mature to RTBC this without taking the performance core gate into account. However this is a one-time thing and while in the critical path, not that relevant compared to things running more often. This at least was my reasoning not to ask for profiling of this - though that was wrong.

I do hope my benchmarks of both the page cache and non-page cached version help alleviate concerns around the performance impact of the library itself.

I do think we should ask upstream to release an official 1.0.0 version of the StackNegotation library, but I don't think this needs to block the commit of this - given its Amsterdam beta target.

To clarify on versions:

The actual negotiation library (https://github.com/willdurand/StackNegotiation) is "willdurand/negotiation": "~1.3", which is a very stable version.

It is _just_ the Stack integration that is tagged 0.1.0.

Both libraries have extensive tests - and I verified some of them.

Thanks!

Adding myself to proposed commit message

Hi! My name is William and I am the "author" of both Negotiation and StackNegotiation.

Negotiation is stable and runs in production for a while now, it has been installed +700k times, and is a class-first citizen in FOSRestBundle, THE REST toolbox for Symfony applications. The API will not change, but internal refactoring may happen to support even more edge cases. Note that this library does not aim at "patching/hidding" incorrect behaviors caused by silly browsers (I am looking at you, IE8).

StackNegotiation works well too, however I never used it in production. That's why I never released a stable version. However, I don't plan to change the API, it works well after all and the integration is rather simple. I can release a stable version and maintain it if you want to rely on stable packages (which makes sense). Let me know.

Also, if you would like to make changes, let's discuss them *before* the release of a stable version :-)

Regards,
William

@william
There is this browser crazyness but sure, we can ship it in Drupal directly.

Thanks, willdurand. If there's no API changes planned for the foreseeable future then having a stable tag to reference in our composer.json would be appreciated. We're used to dealing with "pseudo-stable" modules in Drupal (a LOT of major modules do that) but it's a practice we try to discourage for Drupal developers, so having a library we depend on using a stable tag would help make that case. :-)

Since we're specifying a tag already here I don't think that's a commit blocker; it's easy enough to switch later if needs be. I am still +1 on commit here.

I didn't follow the whole discussion but it is worth mentioning that this middleware does not provide any way to configure fallbacks/priorities yet, which could be interesting. WDYT? Also, it requires at least PHP 5.4.

Priorities are now supported. As for "fallback", it should probably not be part of this middleware.

Drupal is >= PHP 5.4 due to dependencies, trait usage and [] syntax and PHP 5.3 is EOL, so that should be fine.

In which cases would we need to use "fallbacks"?

I think if we can plan a stable release of StackNegotiation once we have this in, that would be appreciated.

Fallback may be useful if it is not possible to find a format based on the accept header data, but this can be handled somewhere else.

@Crell surely

What URLs something lives at is off topic for this thread.

Isn't that the entire topic of this thread when it comes down to it? I'm really fed up with these blanket, contentless statements that attempt to shut down valid discussion.

I brought up the pre-page-cache negotiation requirement as a performance issue, as well as the additional complexity for external caches on #1855260: Page caching broken by accept header-based routing based on the original WSCCI plans, months before those features were implemented. So it is extremely disappointing to be reading this issue discuss all of that over again nearly two years later, without any of that original discussion ever having been "on topic" while this was implemented, and still with no viable plan on how to resolve the issues for external page caches. Vary: Accept is not viable as shown here and in previous discussions.

Putting all of them at different URIs is not reasonable, so just splitting HTML and "not HTML" off to different URIs won't solve anything cache-related.

Putting the different response formats at different URIs would be fine with me, there's not really an argument against it except it not matching REST - but examples of genuine RESTful implementations that successfully resolve these issues have been completely lacking throughout this process.

No, catch, it's not. We could eliminate the need for conneg entirely if and only if we completely and utterly ignore the Accept* and Content Type headers in all requests and force every single different format of every resource to have its own distinct URI. That is, not follow HTTP, revert back to D7 logic and remove the word "REST" from any and all of our self-description (because to do otherwise would frankly be lying).

This issue is about, solely, improving the conneg we have (we already have a conneg library and have for around 2.5 years) and fixing the API around it (making $request->getRequestFormat() reliable, which right now it is not). Ripping out the concept of content negotiation entirely (which is what the discussion of moving every resource to its own URI is) is not on topic for this issue. At all.

I've bumped #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use to critical, answering that should really should have been a pre-requisite to #2019123: Use the same canonical URI paths as for HTML routes happening rather than punted down the line against objections at the time.

So long as HEAD performs format negotiation based on an Accept header, I'm reclassifying this as a bug, because:
- Not parsing the Accept header to the RFC spec is a bug
- Having $request->getRequestFormat() return the wrong answer based on when it's invoked is a bug

Per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?, normal priority bug fixes are still allowed during the beta phase, so long as their value exceeds their cost.

Since there's discussion on #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use about potentially removing header-based format negotiation entirely, I think we should postpone this on a resolution of that discussion, but I'm leaving that decision to catch.

At absolute minimum we should do as #95 suggests and just move the code around. That's a straight up "take an inconsistent API that makes little sense and make it consistent and make sense" task, which should be uncontroversial, even if it has no change in the quality of the accept negotiation itself.

Even if we were to rip out Accept header negotiation entirely (which I think would be a huge mistake), the fallback would still use the _format key on the request to track the format in question (even if that value comes from the URI), and so we need getRequestFormat() to be reliable and predictable.

How much human effort would it be to replace the current broken conneg with willdurand/Negotation ? If not a lot, why don't we do that to get a non-broken negotation that barely affects performance as it fires after the page cache. Then, as things resolve, perhaps add stackneg or perhaps not.

Re #134:

+++ b/core/includes/bootstrap.inc
@@ -307,10 +307,10 @@ function drupal_get_filename($type, $name, $filename = NULL) {
 function drupal_page_cache_get_cid(Request $request) {
-  $cid_parts = array(
+  $cid_parts = [
     $request->getUri(),
-    \Drupal::service('content_negotiation')->getContentType($request),
-  );
+    $request->getRequestFormat(),
+  ];

Per above, conneg is already happening before page cache in HEAD, so can't remove that so long as we're serving different representations at the same URL, so no way to switch to a slower implementation without taking the hit.

How much human effort would it be to replace the current broken conneg with willdurand/Negotation ? If not a lot, why don't we do that to get a non-broken negotation that barely affects performance as it fires after the page cache. Then, as things resolve, perhaps add stackneg or perhaps not.

We can work moving the current implementation into a middleware. This allows us to fix bugs. We can then decide later whether its okay to accept the performance, especially because you can swap it out easily in contrib.

#136 sounds like a good plan.

#136: Can we do that as another issue, please however, to leave this one to wildurand/negotiation / StackNeg?

What are the performance issues you're talking about?

There a no real "performance issues", because as the above benchmarks show the overhead of the library and moving to stack is 0.5 - 1 ms. That is why we want to move to stack first, then exchange the library. That will allow us to judge the overhead better.

The path to the page-cache (similar to symfony HttpCache), is critical however and takes 10 ms currently. It took 2 ms in the previous Drupal 7 version and the reason to optimize is not only raw speed, but also how many clients you can serve from that database cache (e.g. raw computing overhead determines scalability).

However going that overall route we need to send Vary: Accept per rest specification (which is bad for reverse proxy cacheability), so we discuss various options in other issues of what we need to be "fast-by-default".

@fabianx
Sure, here is the other issue #2369681: Move ContentNegotiation into a middleware

I guess this is postponed on #2369681: Move ContentNegotiation into a middleware now...

Per discussion in #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use, we won't be going this route. Sad panda.