Skip to content

Commit

Permalink
bug #32348 [HttpFoundation] Accept must take the lead for Request::ge…
Browse files Browse the repository at this point in the history
…tPreferredFormat() (dunglas)

This PR was squashed before being merged into the 4.4 branch (closes #32348).

Discussion
----------

[HttpFoundation] Accept must take the lead for Request::getPreferredFormat()

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Follow up PR to #32344: if both `Accept` and `Content-Type` are defined, `Accept` must take the lead because it explicitly tells what format the client expect as a response.

Before:

```
$ curl -H 'Accept: application/json' -H 'Content-Type: text/xml' -i 'https://127.0.0.1:8000/userinfo'

[snip]
content-type: text/xml
```

After:

```
$ curl -H 'Accept: application/json' -H 'Content-Type: text/xml' -i 'https://127.0.0.1:8000/userinfo'

[snip]
content-type: application/json
```

Actually, I'm not sure that inferring the content type of the response using the `Content-Type` provided for the request body is a good idea. The HTTP RFC explicitly states that `Accept` must be used to hint a preferred response format (`Content-Type` on the request indicates the type of associated its the body). I would be in favor of being more conservative: use `Accept` if provided (a best practice anyway), and fallback to the default value (HTML by default) otherwise. WDYT?

Commits
-------

60d997d [HttpFoundation] Accept must take the lead for Request::getPreferredFormat()
  • Loading branch information
fabpot committed Jul 4, 2019
2 parents 9b4f801 + 60d997d commit b7c9fcf
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 11 deletions.
21 changes: 14 additions & 7 deletions src/Symfony/Component/HttpFoundation/Request.php
Expand Up @@ -1347,6 +1347,8 @@ public function setFormat($format, $mimeTypes)
* * _format request attribute
* * $default
*
* @see getPreferredFormat
*
* @param string|null $default The default format
*
* @return string|null The request format
Expand Down Expand Up @@ -1563,22 +1565,27 @@ public function isNoCache()
return $this->headers->hasCacheControlDirective('no-cache') || 'no-cache' == $this->headers->get('Pragma');
}

/**
* Gets the preferred format for the response by inspecting, in the following order:
* * the request format set using setRequestFormat
* * the values of the Accept HTTP header
* * the content type of the body of the request.
*/
public function getPreferredFormat(?string $default = 'html'): ?string
{
if (null !== $this->preferredFormat) {
return $this->preferredFormat;
}

$this->preferredFormat = $this->getRequestFormat($this->getContentType());

if (null === $this->preferredFormat) {
foreach ($this->getAcceptableContentTypes() as $contentType) {
if (null !== $this->preferredFormat = $this->getFormat($contentType)) {
break;
}
$preferredFormat = null;
foreach ($this->getAcceptableContentTypes() as $contentType) {
if ($preferredFormat = $this->getFormat($contentType)) {
break;
}
}

$this->preferredFormat = $this->getRequestFormat($preferredFormat ?: $this->getContentType());

return $this->preferredFormat ?: $default;
}

Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
Expand Up @@ -407,14 +407,14 @@ public function testGetPreferredFormat()
$this->assertSame('json', $request->getPreferredFormat('json'));

$request->setRequestFormat('atom');
$request->headers->set('Content-Type', 'application/json');
$request->headers->set('Accept', 'application/xml');
$request->headers->set('Accept', 'application/ld+json');
$request->headers->set('Content-Type', 'application/merge-patch+json');
$this->assertSame('atom', $request->getPreferredFormat());

$request = new Request();
$request->headers->set('Content-Type', 'application/json');
$request->headers->set('Accept', 'application/xml');
$this->assertSame('json', $request->getPreferredFormat());
$request->headers->set('Content-Type', 'application/json');
$this->assertSame('xml', $request->getPreferredFormat());

$request = new Request();
$request->headers->set('Accept', 'application/xml');
Expand Down
Expand Up @@ -504,6 +504,7 @@ public function testPrepareSetContentType()
$response = new Response('foo');
$request = Request::create('/');
$request->setRequestFormat('json');
$request->headers->remove('accept');

$response->prepare($request);

Expand Down

0 comments on commit b7c9fcf

Please sign in to comment.