Problem/Motivation

Drupal 8 includes a nested matcher for filtering routes based on the characteristics of an incoming request. As part of the nested matcher process, several partial matchers are applied to the initial RouteCollection (which can include hundreds of routes in the context of Drupal). One example of partial matcher is the HttpMethodMatcher which looks at the method of the HTTP request and discards any route not supporting that method (routes can specify which method they support in their requirements parameter).

Crell suggested to implement another partial matcher for MIME types specified in the HTTP Accept header. Routes can define requirements which are used in the nested matcher. Here is a couple of routes defining the format they support in an implementation of hook_router_info():

  $route = new Route('conneg/test1', array(
    '_controller' => '\Drupal\conneg\ConnegControllers::test1_html',
  ), array(
    '_format' => 'html',
  ));
  $collection->add('conneg_1_html', $route);

  $route = new Route('conneg/test1', array(
    '_controller' => '\Drupal\conneg\ConnegControllers::test1_json',
  ), array(
    '_format' => 'json',
  ));
  $collection->add('conneg_1_json', $route);

Note how both routes share the same path, but they point to a different controller based on the format they require.

Given that several MIME types are usually part of an HTTP request, and each of them can be assigned a quality (aka weight), routes should as a result be given a weight. Symfony does not allow to weigh routes other than using the order they are ->add()'ed to a RouteCollection, so this might require us to use a custom collection first in the nested matcher and then add all routes in the final RouteCollection from that array, in the right order.

Proposed resolution

Implement a new PartialMatcher class and register it in the DIC in such as a way that it is included in the NestedMatcher process.

Remaining tasks

Review approach used in patch.

User interface changes

none.

API changes

New argument '_format' in the requirements of a route (similar to '_method').

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scor’s picture

Issue summary: View changes

add note about route order

scor’s picture

Status: Active » Needs work
FileSize
6.66 KB

Here is my initial patch based on discussions during BADCamp with @Crell. For now it implements a crappy 'weak routes' array in place of the ordered route collection where routes which are matching because they don't specify a format or match because of */* are added. These are added to the RouteCollection in the end so they have a lower priority.

TODO: switch to the Symfony mime type groups defined in Request::initializeFormats().

At this point reviews on the general directions are welcome.

Crell’s picture

This looks good on general direction. Let's go ahead and use the Response class's mapper for now, then we can swap to the new conneg library when that exists.

scor’s picture

Status: Needs work » Needs review
FileSize
7.51 KB

Added support for Symfony's built-in formats with associated MIME types, and new test for an alternative JSON Accept header.

RouteNotFoundException will have to be replaced with UnsupportedMediaTypeHttpException once #1831074: Send 415 Unsupported Media Type http header when serializer encounters an unsupported format and #1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer are fixed.

Status: Needs review » Needs work

The last submitted patch, 1833440_3_mimetype_matcher.patch, failed testing.

scor’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Routing/MimeTypeMatcher.php
@@ -0,0 +1,76 @@
+    // Generates a list of Symfony formats matching the acceptable MIME types.
+    foreach ($acceptable_mime_types as $mime_type) {
+      if ($request->getFormat($mime_type)) {
+        $acceptable_formats[] = $request->getFormat($mime_type);
+      }
+    }
+    $acceptable_formats = array_unique($acceptable_formats);

I tried using array_filter() here using Request::getFormat() as callable, but couldn't figure it out. something like:

$acceptable_formats = array_filter($acceptable_mime_types, Request::getFormat());
Crell’s picture

If you want to use a method of an object as a callable, make it an array:

$acceptable_formats = array_filter($acceptable_mime_types, array($request, 'getFormat'));

Crell’s picture

Fortunately the bugs in the previous patch were easy to fix, and I did some other tidying while I was in there. This is passing all of the Routing tests for me locally.

scor’s picture

Thanks @crell for the fixes. following up with some additional test coverage to keep up with the tests of HttpMethodMatcher

Crell’s picture

Status: Needs review » Reviewed & tested by the community

If the bot's happy, I'm happy. Mr. Roboto will tell us if he's not happy.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1833440_8_mimetype_matcher.patch, failed testing.

scor’s picture

Status: Needs work » Needs review
FileSize
8.44 KB

obviously I should not make supposedly trivial changes without re-running the tests. fixed MimeTypeMatcherTest.php to include the right exception.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, Mr. Roboto.

klausi’s picture

Looks good to me as well, made some minor documentation fixes.

klausi’s picture

Crell’s picture

Priority: Normal » Major

Since this is a blocker for the full on REST support, I'm bumping priority.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

scor’s picture

Follow up issue for finer partial matcher: #1844920: Add weight to routes

scor’s picture

Status: Postponed » Needs review

Given that #1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer was committed, this patch can now be tested/reviewed.

scor’s picture

this patch also removes the @todo comment :)

klausi’s picture

Improved use statement ordering, removed unused use statements.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Yay, correct exceptions.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

input format quirk