Problem/Motivation

https://tools.ietf.org/html/rfc7231#section-6.5.6 says

6.5.6.  406 Not Acceptable

   The 406 (Not Acceptable) status code indicates that the target
   resource does not have a current representation that would be
   acceptable to the user agent, according to the proactive negotiation
   header fields received in the request (Section 5.3), and the server
   is unwilling to supply a default representation.

   The server SHOULD generate a payload containing a list of available
   representation characteristics and corresponding resource identifiers
   from which the user or user agent can choose the one most
   appropriate.  A user agent MAY automatically select the most
   appropriate choice from that list.  However, this specification does
   not define any standard for such automatic selection, as described in
   Section 6.4.1.

… i.e. there's no standardized way to communicate the list of acceptable formats. Also see https://httpstatuses.com/300. See https://github.com/Respect/Rest/issues/39 for an example discussion of a project attempting to implement this … and getting sucked into an endless discussion. That being said, I think we should do something until it's standardized.

Proposed resolution

Update \Drupal\Core\Routing\RequestFormatRouteFilter

Remaining tasks

TBD

User interface changes

TBD

API changes

TBD

Data model changes

None.

CommentFileSizeAuthor
#19 2955383-19.patch6.61 KBwim leers
#19 interdiff.txt1.1 KBwim leers
#17 2955383-17.patch6.52 KBwim leers
#17 interdiff.txt2.39 KBwim leers
#16 2955383-16.patch5.46 KBwim leers
#6 2955383-6.patch8.91 KBwim leers
#6 interdiff.txt865 byteswim leers
#5 2955383-5.patch8.12 KBwim leers
#5 interdiff.txt4.87 KBwim leers
#2 2955383-2.patch3.97 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2955383-2.patch3.97 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs tests

Status: Needs review » Needs work

The last submitted patch, 2: 2955383-2.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt4.87 KB
new 2955383-5.patch8.12 KB

This should be green.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs tests
Related issues: +#2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't
StatusFileSize
new interdiff.txt865 bytes
new 2955383-6.patch8.91 KB

And here's minimal integration test coverage. Once #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't is in, it can become nicer/better.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented

On iPad, so can’t easily quote the code or run tests, but don’t see why the change to UnroutedUrlAssembler is needed? The change to RequestFormatRouteFilter looks *really* nice.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks for the review!

but don’t see why the change to UnroutedUrlAssembler is needed?

Because Url::fromUri('http://example.com/foo?_format=foobar', ['query' => ['_format' => 'json']])->toString(TRUE)->getGeneratedUrl()
returns this in HEAD:

http://example.com/foo?_format=foobar&_format=json

the changes fix a bug so that it returns what you'd expect instead:

http://example.com/foo?_format=json

(Yes, that should be moved into a separate issue probably.)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work

#8, I don't see the difference in both examples here.

  1. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
@@ -52,7 +54,18 @@ public function filter(RouteCollection $collection, Request $request) {
+        $content_type = $request->getMimeType($available_format);
+        $links[] = "<$url>; rel=\"alternate\"; type=\"$content_type\"";

    So, is this thing is content type or a mime type? I think we should rename the $content_type variable here to $mime_type. 

  2. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
@@ -80,4 +93,11 @@ protected static function getDefaultFormat(RouteCollection $collection) {
+  protected static function getAvailableFormats(RouteCollection $collection) {

    This should get some documentation.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

I don't see the difference in both examples here.

🤐😳 — fixed!

Agreed with both of your review points. But I first want sign-off on the approach, it's no time to nitpick yet!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I think I agree with your assesment in #8, that the changes to the unrouted url assembler should be moved to a different issue. It's a different bug and is probably going to require more discussion than this bugfix.

I'm not sure if adding this as an additional header is the best solution, maybe a response with this in _links. However, that might be very hal specific. I see that is one of the suggestions in the github issue as well: https://github.com/Respect/Rest/issues/39#issuecomment-6495329

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Related issues: +#2955685: Unrouted URLs cannot have have overridden query or fragments

Created #2955685: Unrouted URLs cannot have have overridden query or fragments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#11:

However, that might be very hal specific.

Exactly. That's a non-option because it only makes sense for HAL normalizations. And this is decidedly not HAL-specific.

#12: thank you for creating that issue! ❤️

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: List available representations in 406 responses » [PP-1] List available representations in 406 responses
Status: Needs review » Postponed

Let's make it clear this is blocked on #2955685: Unrouted URLs cannot have have overridden query or fragments.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] List available representations in 406 responses » List available representations in 406 responses
Assigned: Unassigned » wim leers
Status: Postponed » Needs work

#2955685: Unrouted URLs cannot have have overridden query or fragments landed!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2955383-16.patch5.46 KB

A simple rebase that removes all changes that landed in #2955685: Unrouted URLs cannot have have overridden query or fragments was all we needed!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
StatusFileSize
new interdiff.txt2.39 KB
new 2955383-17.patch6.52 KB 
+++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
@@ -95,4 +107,11 @@ protected static function getDefaultFormat(RouteCollection $collection) {
+  protected static function getAvailableFormats(RouteCollection $collection) {

Missing comment. Fixed.

Better yet: #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't added very similar logic … so we can now just extract that existing logic into a separate function instead!

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1492,6 +1492,8 @@ protected function assert406Response(ResponseInterface $response) {
+      $this->stringContains('?_format=', $response->getHeader('Link'));

I'm curious whether we could test for the formats as well here.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt1.1 KB
new 2955383-19.patch6.61 KB

Done!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

#19 fixes #18 and I couldn't find anything else either. The patch has good testcoverage and it should make everyone's lives easier.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 8ce2a3e and pushed to 8.6.x. Thanks!

Credited @borisson_ and @dawehner for reviews.

  • alexpott committed 8ce2a3e on 8.6.x 
    Issue #2955383 by Wim Leers, borisson_, dawehner: List available...

Status: Fixed » Closed (fixed)

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