Accessing a non existing format results in a 500 [#2594777]

Problem/Motivation

Enable rest
Enable serialize
Access http://d8.dev/node/3?_format=banana

Result:

The website encountered an unexpected error. Please try again later.undefined</br>undefined</br>undefined<em class="placeholder">Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException</em>: Not acceptable in undefined<em class="placeholder">Drupal\Core\EventSubscriber\AcceptNegotiation406-&gt;onViewDetect406()</em> (line undefined<em class="placeholder">37</em> of undefined<em class="placeholder">core/lib/Drupal/Core/EventSubscriber/AcceptNegotiation406.php</em>). undefined<pre class="backtrace">Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher-&gt;dispatch(&#039;kernel.view&#039;, Object)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache-&gt;fetch(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache-&gt;lookup(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1)
Stack\StackedHttpKernel-&gt;handle(Object, 1, 1)
Drupal\Core\DrupalKernel-&gt;handle(Object)
</pre>

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#28	DefaultExceptionSubscriber_php_-_www_-____Sites_drupal_d8_www_.png	35.08 KB	clemens.tolboom
#26	interdiff-18-26.txt	1.85 KB	therealssj
#26	2594777-26.patch	3.66 KB	therealssj
#26	8.0.x: PHP 5.5 & MySQL 5.5 14,610 pass
#23	2594777-23.patch	3.65 KB	therealssj
#23	8.0.x: PHP 5.5 & MySQL 5.5 14,609 pass, 1 fail
#23	interdiff-18-23.txt	1.2 KB	therealssj
#18	interdiff.txt	1.9 KB	Wim Leers
#18	2594777-18.patch	3.63 KB	Wim Leers
#18	8.0.x: PHP 5.5 & MySQL 5.5 14,491 pass
#12	interdiff.txt	1.83 KB	dawehner
#12	2594777-12.patch	3.57 KB	dawehner
#12	8.0.x: PHP 5.5 & MySQL 5.5 14,488 pass
#11	interdiff.txt	1.52 KB	dawehner
#11	2594777-11.patch	1.74 KB	dawehner
#11	8.0.x: PHP 5.5 & MySQL 5.5 14,480 pass
#6	2594777.patch	1.58 KB	damiankloip
#6	8.0.x: PHP 5.5 & MySQL 5.5 14,475 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

16 October 2015 at 13:15

dawehner created an issue. See original summary.

Comment #2

dawehner

German

CreditAttribution: dawehner as a volunteer commented 16 October 2015 at 13:20

Title:

Enable rest without hal_json and accessing ?_format=hal_json results in a 406

» Enable rest without hal_json and accessing ?_format=hal_json results in a 500

Comment #3

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom as a volunteer commented 17 October 2015 at 09:46

[edit]~~It's even worse.~~ Visiting http://drupal.d8/node/1?_format=whatever throws the same exception. (doh)[/edit]

There should not be a PHP exception induced by bad formats.

Comment #4

damiankloip CreditAttribution: damiankloip commented 17 October 2015 at 20:51

This returns a 406 response for me, which I think is fine. I just think the problem might be that we have the trace there?

Comment #5

damiankloip CreditAttribution: damiankloip commented 17 October 2015 at 20:56

So the problem is that DefaultExceptionSubscriber is handling this I guess. Because the exception implements HttpExceptionInterface the correct response code is set, but the trace is always added to the response content.

Comment #6

damiankloip CreditAttribution: damiankloip commented 17 October 2015 at 21:27

File	Size
2594777.patch	1.58 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,475 pass

It's rough, but maybe we need to do something like this for formats we don't know about?

Comment #7

dawehner

German

CreditAttribution: dawehner as a volunteer commented 19 October 2015 at 01:18

Title:

Enable rest without hal_json and accessing ?_format=hal_json results in a 500

» Accessing a non existing format results in a 500

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 29 January 2016 at 14:15

Status:

Active

» Needs review

NR for #6.

Comment #9

dawehner

German

CreditAttribution: dawehner as a volunteer commented 30 January 2016 at 15:33

Status:	Needs review	» Needs work
Issue tags:		+Needs tests

This is looking great but I guess we better have some kind of integration test coverage

Comment #10

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting, Chapter Three commented 1 February 2016 at 18:14

Working on the tests now.

Comment #11

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting, Chapter Three commented 1 February 2016 at 18:33

Status:	Needs work	» Needs review
Issue tags:	-Needs tests

File	Size
2594777-11.patch	1.74 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,480 pass
interdiff.txt	1.52 KB

Here is one. Sadly shit actually just works if you test it.

Comment #12

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting, Chapter Three commented 1 February 2016 at 18:42

File	Size
2594777-12.patch	3.57 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,488 pass
interdiff.txt	1.83 KB

Here is the test.

Comment #13

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting, Chapter Three commented 5 February 2016 at 09:25

Priority:

Normal

» Major

Being able to trigger 500s easily on any page is IMHO major for me.

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 5 February 2016 at 09:50

Status:	Needs review	» Reviewed & tested by the community
Issue tags:		+DX (Developer Experience), +API-First Initiative

Comment #15

damiankloip CreditAttribution: damiankloip commented 5 February 2016 at 11:05

Status:

Reviewed & tested by the community

» Needs work

Sorry, small thing.

+++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
@@ -173,11 +187,18 @@ protected function onJson(GetResponseForExceptionEvent $event) {
     // If it's an unrecognized format, assume HTML.

I guess we need to change that comment now too.

Otherwise, looks like some nice changes to my patch. Thanks! After that, agree it is RTBC.

Comment #16

damiankloip CreditAttribution: damiankloip commented 5 February 2016 at 11:09

Also, as with the example in the summary. It is not immediately clear that any request for a non-existent format fails. hal_json *can* exist. E.g. the same outcome would occur with a request like http://d8-dev/node/1?_format=bananas Not sure if the tests should reflect that too? As it's easy to get confused as to why hal_json wouldn't be a valid format IMO.

Comment #17

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 5 February 2016 at 11:57

Assigned:

Unassigned

» Wim Leers

+1 for #16.

I'll address the feedback in a reroll.

Comment #18

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 5 February 2016 at 12:01

Assigned:	Wim Leers	» Unassigned
Status:	Needs work	» Needs review

File	Size
2594777-18.patch	3.63 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,491 pass
interdiff.txt	1.9 KB

Comment #19

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting, Chapter Three commented 5 February 2016 at 12:02

Status:

Needs review

» Reviewed & tested by the community

+1. Works for me

Comment #20

dawehner

German

CreditAttribution: dawehner as a volunteer and at Tag1 Consulting, Chapter Three commented 5 February 2016 at 12:02

Issue summary:

View changes

Comment #21

3 February 2016 at 18:06

catch committed 4c643ff on

Issue #2594777 by dawehner, Wim Leers, damiankloip: Accessing a non...

Comment #22

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 10 February 2016 at 00:26

Status:

Reviewed & tested by the community

» Patch (to be ported)

Committed/pushed to 8.1.x, thanks!

For 8.0.x, we probably need to explicitly @internal the new method and underscore prefix it, so moving to 'to be ported' for that.

Comment #23

therealssj CreditAttribution: therealssj commented 3 April 2016 at 05:43

File	Size
interdiff-18-23.txt	1.2 KB
2594777-23.patch	3.65 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,609 pass, 1 fail

A Patch for 8.0.x in accordance with changes mentioned in #22

Comment #24

therealssj CreditAttribution: therealssj commented 3 April 2016 at 05:44

Status:

Patch (to be ported)

» Needs review

Comment #25

3 April 2016 at 06:22

Status:

Needs review

» Needs work

The last submitted patch, 23: 2594777-23.patch, failed testing.

Comment #26

therealssj CreditAttribution: therealssj commented 3 April 2016 at 06:38

Status:

Needs work

» Needs review

File	Size
2594777-26.patch	3.66 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,610 pass
interdiff-18-26.txt	1.85 KB

2 files were hidden/shown/deleted

File	Size
interdiff-18-23.txt	1.2 KB
2594777-23.patch	3.65 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,609 pass, 1 fail

Patch for 8.x
Tested.

Comment #27

dawehner

German

CreditAttribution: dawehner as a volunteer and at Chapter Three, Tag1 Consulting commented 3 April 2016 at 09:40

Status:

Needs review

» Reviewed & tested by the community

Well, I'm not sure whether we still care about 8.0.x at that point, but sure, this change looks right.

Comment #28

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom as a volunteer commented 3 April 2016 at 10:00

File	Size
DefaultExceptionSubscriber_php_-_www_-____Sites_drupal_d8_www_.png	35.08 KB

This does not work for 8.2.x as I still get a 500

The exception is definitely not

// core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php:194
if ($exception instanceof HttpExceptionInterface) {

Comment #29

therealssj CreditAttribution: therealssj commented 3 April 2016 at 10:09

I just tried to reproduce this is 8.2.x and i don't see any issue.
@clemens.tolboom could you tell the steps you followed to get that 500?

Comment #30

dawehner

German

CreditAttribution: dawehner as a volunteer and at Chapter Three, Tag1 Consulting commented 3 April 2016 at 10:57

@clemens.tolboom
Please open up a new issue ...

Comment #31

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom as a volunteer commented 3 April 2016 at 12:34

Sorry for wasting your time. I had a view not liking the format. That's another issue (I hope)

Comment #32

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 5 April 2016 at 18:52

Version:	8.0.x-dev	» 8.1.x-dev
Status:	Reviewed & tested by the community	» Closed (fixed)

8.0.x has entered it's last RC so this is no longer eligible for commit.

Accessing a non existing format results in a 500

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Referenced by