Remove ContentNegotiation and embed functionality in the middleware

Is there an actual usecase in replacing it? I mean its something you should actually not replace, given how hacky it is already. I'd honestly rather expected people to replace the middleware entirely.

Seeing that error lazy me would have just extended the class and overrode the methods... Daniel is right though, if you really need to replace the functionality I'd error on replacing the middleware.

+++ /dev/null
@@ -1,47 +0,0 @@
-<?php
-
-/**
- * @file
- * Contains \Drupal\accept_header_routing_test\AcceptHeaderMiddleware.
- */
-
-namespace Drupal\accept_header_routing_test;
-
-use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpKernel\HttpKernelInterface;
-
-/**
- * Example implementation of accept header based content negotation.
- */
-class AcceptHeaderMiddleware implements HttpKernelInterface {
-
-  /**
-   * Constructs a new AcceptHeaderMiddleware instance.
-   *
-   * @param \Symfony\Component\HttpKernel\HttpKernelInterface $app
-   *   The app.
-   */
-  public function __construct(HttpKernelInterface $app) {
-    $this->app = $app;
-  }
-
-  /**
-   * {@inheritdoc}
-   */
-  public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
-    $mapping = [
-      'application/json' => 'json',
-      'application/hal+json' => 'hal_json',
-      'application/xml' => 'xml',
-      'text/html' => 'html',
-    ];
-
-    $accept = $request->headers->get('Accept') ?: ['text/html'];
-    if (isset($mapping[$accept[0]])) {
-      $request->setRequestFormat($mapping[$accept[0]]);
-    }
-
-    return $this->app->handle($request, $type, $catch);
-  }
-
-}

+++ b/core/modules/system/tests/modules/accept_header_routing_test/src/CustomContentNegotiation.php
@@ -0,0 +1,55 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\accept_header_routing_test\CustomContentNegotiation.
+ */
+
+namespace Drupal\accept_header_routing_test;
+
+use Drupal\Core\ContentNegotiationInterface;
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * Provides content negotiation based upon query parameters and the accept header.
+ */
+class CustomContentNegotiation implements ContentNegotiationInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getContentType(Request $request) {
+    // AJAX iframe uploads need special handling, because they contain a JSON
+    // response wrapped in <textarea>.
+    if ($request->get('ajax_iframe_upload', FALSE)) {
+      return 'iframeupload';
+    }
+
+    if ($request->query->has('_format')) {
+      return $request->query->get('_format');
+    }
+
+    // Create accept header map.
+    $type_map = [
+      'application/json' => 'json',
+      'application/hal+json' => 'hal_json',
+      'application/xml' => 'xml',
+      'text/html' => 'html',
+    ];
+
+    // Get the first accept header.
+    $accept = explode(',', $request->headers->get('Accept'));
+
+    // Check to see if the accept header is in out list.
+    if (isset($type_map[$accept[0]])) {
+      return $type_map[$accept[0]];
+    }
+
+    if ($request->isXmlHttpRequest()) {
+      return 'ajax';
+    }
+
+    // Do HTML last so that it always wins.
+    return 'html';
+  }
+}

what is all this?

@Jaesin
Do you mind removing the test code changes? I think a real accept header based middleware should be a middleware and not need that additional level of indirection.

So, regarding overriding the entire middleware (@dawehner + @neclimdul) versus just overriding the ContentNegotiation service:

• It can be argued both ways.
• I think micro performance matters greatly in this case, so I wonder if the reason no interface exists already is to avoid that one additional OpCache retrieval?
• Services — private or not — can be overridden. And the rule is that any service has an interface, so it can be overridden.
• Therefore I see two valid solutions:
  1. Allow the http_negotiation.format_negotiator service to be overridden: Add an interface to the service (i.e. the current patch).
  2. Only allow the http_middleware.negotiation middleware service to be overridden: Remove ContentNegotiation's logic and move it into NegotiationMiddleware. It seems they're tightly coupled anyway, and the complexity of ContentNegotiation is so small that it does actually make the overall logic simpler.

Re #8: The reason no interface exists is because the ContentNegotiation class was never expected to live this long. :-) It was added in a hacky form 3 years ago and we've tried to kill it about 4 times. It's a tricky bugger. Opcache micro-optimization was never a consideration.

Given how trivial ContentNegotiation is in its current form, I'd be in favor of #8.2: Just kill it finally and move it into the middleware. We can probably just move the getContentType() method itself to the middleware class and be done with it.

Given how trivial ContentNegotiation is in its current form, I'd be in favor of #8.2: Just kill it finally and move it into the middleware. We can probably just move the getContentType() method itself to the middleware class and be done with it.

+1 to do that. For me ContentNegotation is really just about helping the middleware, its kinda a private method.

Yep, makes sense :)

Updating IS accordingly. Agreed that this is a good Novice candidate so giving a precise outline. Also, beta eval.

I made this update. Please review.

Forgot to add my interdiff.txt file.

This issue needs a new title.

@Jaesin: I removed spaces.

Looks good in general.

+++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
@@ -81,5 +69,32 @@ public function registerFormat($format, $mime_type) {
+   *   The request object from which to extract the content type.
...
+  public function getContentType(Request $request) {

This no longer needs to be public.

.

+++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
@@ -82,7 +82,7 @@ public function registerFormat($format, $mime_type) {
+  private function getContentType(Request $request) {

Let's make it protected

Thank you

Since this has been around for 3 years I guess we need a CR to tell people to just override the middleware in order to do their own content negotiation.

+++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
@@ -0,0 +1,19 @@
+class NegotiationMiddlewareTest extends UnitTestCase {
+
+}

???

NM, I see. They where removed because the method wasn't public. re-adding tests. Also some more tests just because I'm a fan of test coverage.

to the testbot. #26 still applies though.

1. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
   @@ -0,0 +1,142 @@
   +   * Test that handle correctly hands off to sub application.
   

   Minor nit: handle(). Otherwise I kept reading "handle correctly" to mean that something else handled something correctly, which is totally not what we're saying here. :-)

2. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
   @@ -0,0 +1,142 @@
   +    $request = $this->prophesize('\Symfony\Component\HttpFoundation\Request');
   

   We can use PHP 5.5 style class naming here now! Huzzah!

3. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
   @@ -0,0 +1,142 @@
   +    $request = $this->prophesize('\Symfony\Component\HttpFoundation\Request');
   

   Here too.

Added one

Let's do this!

(Side note: ::class is a constant, so it evaluates to the class name at compile time, not runtime. There shouldn't be any difference at runtime between a string literal and a constant, or if so it's so small that you wouldn't even notice.)

Can't reproduce. Might be a rouge testbot.

yeah, there was some problem with opcode caching something something. back to previous status.

+1 for the patch

This makes sense - if you want to swap out the negotiation layer you'll need to swap out http_middleware.negotiation - the http_negotiation.format_negotiator is just noise. This patch is just followup from all the work in this area.

Committed f29f1e2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Published the change record.