Problem/Motivation

While working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, I noticed that responses to DELETE requests correctly have status 204, correctly have no body, but incorrectly have a Content-Type header, and it's even set tot he completely nonsensical text/html; charset=UTF-8. See https://tools.ietf.org/html/rfc2616#section-10.2.5 and #2737719-94: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

The root cause: \Symfony\Component\HttpFoundation\Response::prepare():

            // Fix Content-Type
            $charset = $this->charset ?: 'UTF-8';
            if (!$headers->has('Content-Type')) {
                $headers->set('Content-Type', 'text/html; charset='.$charset);
            } elseif (0 === stripos($headers->get('Content-Type'), 'text/') && false === stripos($headers->get('Content-Type'), 'charset')) {
                // add the charset
                $headers->set('Content-Type', $headers->get('Content-Type').'; charset='.$charset);
            }

Nope, this was fixed a long time ago: https://github.com/symfony/symfony/issues/12744 + https://github.com/symfony/symfony/commit/75abd1a451d97f5075128ea10b2598....

The real root cause is PHP: https://bugs.php.net/bug.php?id=70189. This bug exists even in PHP 7.0 (at least in 7.0.12). The fix was committed in March 2016, but it either didn't make it to PHP 7, or it didn't actually solve the problem.

I can reproduce this both with the CLI (when running PHPUnit), and when accessing Drupal via Apache (when performing actual DELETE requests to an actual Drupal instance).

Proposed resolution

Fix the logic.
Wait for PHP to be fixed, then make the tests more strict.

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#11 2821711-11.patch1.25 KBWim Leers
#11 interdiff.txt7.42 KBWim Leers
#4 2821711-4.patch1.26 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer and at Acquia commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer and at Acquia commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [upstream] REST module returns 204 response to DELETE request, with Content-Type: text/html — PHP is always setting it » Apache always sets Content-Type: text/html, even for DELETE requests
Status: Postponed » Needs review
FileSize
2821711-4.patch1.26 KB

I looked into this again, and wanted to make sure this is not a PHP problem.

I created:

test.php
<?php
http_response_code(204);
test.html


(empty file)

Then I performed DELETE requests to both:

$ curl --verbose -X DELETE http://d8/test.php
*   Trying ::1...
* TCP_NODELAY set
* Connected to d8 (::1) port 80 (#0)
> DELETE /test.php HTTP/1.1
> Host: d8
> User-Agent: curl/7.51.0
> Accept: */*
> 
< HTTP/1.1 204 No Content
< Date: Fri, 27 Jan 2017 15:34:04 GMT
< Server: Apache/2.4.23 (Unix) PHP/7.0.12
< X-Content-Type-Options: nosniff
< X-Powered-By: PHP/7.0.12
< Content-Length: 0
< Content-Type: text/html; charset=UTF-8
< 
* Curl_http_done: called premature == 0
* Connection #0 to host d8 left intact
wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ curl --verbose -X DELETE http://d8/test.html
*   Trying ::1...
* TCP_NODELAY set
* Connected to d8 (::1) port 80 (#0)
> DELETE /test.html HTTP/1.1
> Host: d8
> User-Agent: curl/7.51.0
> Accept: */*
> 
< HTTP/1.1 405 Method Not Allowed
< Date: Fri, 27 Jan 2017 15:34:53 GMT
< Server: Apache/2.4.23 (Unix) PHP/7.0.12
< X-Content-Type-Options: nosniff
< Allow: GET,HEAD,POST,OPTIONS,TRACE
< Content-Length: 233
< Content-Type: text/html; charset=iso-8859-1
< 
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>405 Method Not Allowed</title>
</head><body>
<h1>Method Not Allowed</h1>
<p>The requested method DELETE is not allowed for the URL /test.html.</p>
</body></html>
* Curl_http_done: called premature == 0
* Connection #0 to host d8 left intact

Both have Content-Type: text/html response headers. Even when I add header_remove('Content-Type'); to test.php, the server still contains a Content-Type: text/html response header.

Also, it's impossible to detect which HTTP server is used from within the PHP CLI. So… simply documenting all this is all we can do for now.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

This look good to me. I like the more verbose explanation. The only thing I was wondering is if we should have an issue that we immediately postpone on the Apache fix and add a @todo. But not sure if we create core issues that are postponed on 3rd party projects. So RTBC

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Documentation

We could create such an issue, but it's likely to linger forever. This has been a default for many, many, many years. It's unlikely to change. So best to just document it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2821711-4.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Random fail in Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Needs work

This is too bad about PHP's behavior.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -911,7 +911,10 @@ public function testDelete() {
+    // sets it to 'text/html' by default. We also cannot detect the presence of
+    // Apache either here in the CLI. In the future. For now having this
+    // documented here is all we can do.

Wim, there is a typo in the comment. There is not a verb in "In the future."

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt7.42 KB
2821711-11.patch1.25 KB

Fixed comment.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

I think that is the wrong interdiff ;-)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

It is. But then again it's pretty silly for a patch this small :) So I hope it's okay like this?

cilefen’s picture

cilefen CreditAttribution: cilefen commented

I tried pinging you on IRC that I wasn't asking you to waste your time on the diff. I'll commit this doc improvement in a bit.

  • cilefen committed c1ec091 on 8.4.x 
    Issue #2821711 by Wim Leers: Apache always sets Content-Type: text/html...

  • cilefen committed ba41feb on 8.3.x 
    Issue #2821711 by Wim Leers: Apache always sets Content-Type: text/html...
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Fixed

Committed c1ec091 and pushed to 8.4.x. Cherry-picked as ba41feb and pushed to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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