[HttpFoundation] Adds getAcceptableFormats() method for Request #26486

AndreiIgna · 2018-03-11T15:51:34Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21909
License	MIT
Doc PR	symfony/symfony-docs#...

Adds a new method getAcceptableFormats() for Request. This reads the content types in Accept header and maps them to formats already defined in Request class

In a request made by a browser, based on the default Accept header, the getAcceptableFormats() will return an array ['html', 'xml']

In progress

gather feedback for my changes
submit changes to the documentation

derrabus · 2018-03-13T11:19:17Z

It looks like this PR contains unrelated commits. Could you do a rebase against master?

AndreiIgna · 2018-03-13T12:21:46Z

Oops used GitHub desktop, which did something unexpected to the rebase. Should be ok now

derrabus · 2018-03-13T12:45:11Z

src/Symfony/Component/HttpFoundation/Request.php

@@ -170,6 +170,11 @@ class Request
     */
    protected $format;

+    /**
+     * @var string


Should be array.

derrabus · 2018-03-13T12:47:48Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

@@ -10,6 +10,7 @@ CHANGELOG
 * The `get()` method of the `AcceptHeader` class now takes into account the
   `*` and `*/*` default values (if they are present in the Accept HTTP header)
   when looking for items.
+ * Adds `getAcceptableFormats()` for reading acceptable formats based on Accept header


Should be added to be consistent with the wording of other entries in that file.

AndreiIgna · 2018-03-15T16:41:34Z

Thanks noticing and letting me know about those @derrabus

The added method makes it easier to check what data formats the clients accepts, ex:

if (in_array('html', $request->getAcceptableFormats())) {
  // return html data, maybe a template
} elseif (in_array('xml', $request->getAcceptableFormats())) {
  // return xml
} else {
  // return json data
}

If this is useful and included in next release, I'll make the addition in documentation repo as well

nicolas-grekas · 2018-03-16T09:47:06Z

src/Symfony/Component/HttpFoundation/Request.php

+            return $this->acceptableFormats;
+        }
+
+        return $this->acceptableFormats = array_values(array_unique(array_filter(array_map(array($this, 'getFormat'), $this->getAcceptableContentTypes()))));


is array_values actually required?

Is added because the mix of array_filter and array_unqiue sometimes returns [0 => 'html', 2 => 'xml', 5 => 'json'].
This create problems with tests or confusion when checking the array, so it's best to reset the keys

nicolas-grekas · 2018-03-16T09:47:52Z

src/Symfony/Component/HttpFoundation/Request.php

+     *
+     * @return array List of acceptable formats by the client
+     */
+    public function getAcceptableFormats()


missing return type (and removal of the corresponding docblock line since it will become useless)

nicolas-grekas · 2018-06-04T19:54:02Z

@AndreiIgna would you mind rebasing and moving the line in the changelog on 4.2?

AndreiIgna · 2018-06-05T08:53:55Z

Yes sure, is updated now

fabpot · 2018-06-11T08:42:33Z

src/Symfony/Component/HttpFoundation/Request.php

+    /**
+     * @var array
+     */
+    protected $acceptableFormats;


should be private

fabpot · 2018-06-11T08:42:41Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

+4.2.0
+-----
+
+ * Adds `getAcceptableFormats()` for reading acceptable formats based on Accept header


AndreiIgna · 2018-06-11T10:56:53Z

Updated based on last review.

While checking in more detail the similar $acceptableContentTypes, saw that it's assigned null on initialize and duplicate methods. Added this for $acceptableFormats too, hope this is ok

nicolas-grekas · 2018-06-19T11:47:48Z

Thank you @AndreiIgna.

…r Request (AndreiIgna) This PR was squashed before being merged into the 4.2-dev branch (closes #26486). Discussion ---------- [HttpFoundation] Adds getAcceptableFormats() method for Request | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21909 | License | MIT | Doc PR | symfony/symfony-docs#...  Adds a new method `getAcceptableFormats()` for `Request`. This reads the content types in Accept header and maps them to formats already defined in `Request` class In a request made by a browser, based on the default Accept header, the `getAcceptableFormats()` will return an array `['html', 'xml']` In progress - [x] gather feedback for my changes - [x] submit changes to the documentation Commits ------- 8a127ea [HttpFoundation] Adds getAcceptableFormats() method for Request

…od (AndreiIgna) This PR was squashed before being merged into the master branch (closes #9898). Discussion ---------- [HttpFoundation] Add info for getAcceptableFormats() method Adds info for `Request::getAcceptableFormats()` Should it add more info on docs page? Ex: `Request::getAcceptableContentTypes` returns the accepted content types which can be `text/html`, `application/json`, etc. `Request::getAcceptableFormats` processes that and returns the formats based on those content types `html`, `json` ref symfony/symfony#26486 Commits ------- 65d100e [HttpFoundation] Add info for getAcceptableFormats() method

Tobion · 2018-10-27T01:48:04Z

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

+
+        $request = new Request();
+        $request->headers->set('Accept', 'text/html, application/xhtml+xml, application/xml;q=0.9, */*');
+        $this->assertEquals(array('html', 'xml'), $request->getAcceptableFormats());


why would it only return html and xml if everything is acceptable according to */*? I think this shows that the method is not clear and generic enough to be added to the core IMO.

I can't see where I would possibly use this method. What would be useful is a similar method that accepts formats as argument and would return the ones that are acceptable according to the accept header. This would then allow to make use of */* and this is what people usually need in REST APIs etc.

nicolas-grekas · 2018-11-01T09:53:05Z

FYI, reverted in #29047

…ethod for Request" (Tobion) This PR was merged into the 4.2-dev branch. Discussion ---------- Revert "[HttpFoundation] Adds getAcceptableFormats() method for Request" This reverts commit 8a127ea. | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #26486 | License | MIT | Doc PR | As I said in #26486 (comment) and people wonder in https://symfony.com/blog/new-in-symfony-4-2-acceptable-request-formats#comment-22747, I don't think this method clear and generic enough to be added to the core. I can't see where I would possibly use this method. What would be useful is a similar method that accepts formats as argument and would return the ones that are acceptable according to the accept header. This would then allow to make use of `*/*` and this is what people usually need in REST APIs etc. But for now, we should revert it before it gets released like this. Commits ------- 397ed83 Revert "[HttpFoundation] Adds getAcceptableFormats() method for Request"

carsonbot added Status: Needs Review HttpFoundation Feature labels Mar 11, 2018

nicolas-grekas added this to the 4.1 milestone Mar 12, 2018

AndreiIgna force-pushed the httpfoundation-request-add-acceptable-formats branch from 08a9337 to 00acc8c Compare March 13, 2018 12:15

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 13, 2018

derrabus requested changes Mar 13, 2018

View reviewed changes

nicolas-grekas reviewed Mar 16, 2018

View reviewed changes

AndreiIgna force-pushed the httpfoundation-request-add-acceptable-formats branch 2 times, most recently from 1e9bf3f to eef572b Compare March 23, 2018 09:38

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

AndreiIgna force-pushed the httpfoundation-request-add-acceptable-formats branch from eef572b to 7f062b1 Compare June 5, 2018 08:28

nicolas-grekas approved these changes Jun 5, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jun 5, 2018

AndreiIgna mentioned this pull request Jun 7, 2018

[HttpFoundation] Add info for getAcceptableFormats() method symfony/symfony-docs#9898

Closed

javiereguiluz approved these changes Jun 11, 2018

View reviewed changes

fabpot reviewed Jun 11, 2018

View reviewed changes

AndreiIgna force-pushed the httpfoundation-request-add-acceptable-formats branch from 7f062b1 to 9bfb693 Compare June 11, 2018 10:52

[HttpFoundation] Adds getAcceptableFormats() method for Request

8a127ea

nicolas-grekas force-pushed the httpfoundation-request-add-acceptable-formats branch from 9bfb693 to 8a127ea Compare June 19, 2018 11:26

nicolas-grekas merged commit 8a127ea into symfony:master Jun 19, 2018

Tobion reviewed Oct 27, 2018

View reviewed changes

Tobion mentioned this pull request Oct 31, 2018

Revert "[HttpFoundation] Adds getAcceptableFormats() method for Request" #29047

Merged

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Adds getAcceptableFormats() method for Request #26486

[HttpFoundation] Adds getAcceptableFormats() method for Request #26486

AndreiIgna commented Mar 11, 2018 •

edited

Loading

derrabus commented Mar 13, 2018

AndreiIgna commented Mar 13, 2018

derrabus Mar 13, 2018

derrabus Mar 13, 2018

AndreiIgna commented Mar 15, 2018

nicolas-grekas Mar 16, 2018

AndreiIgna Mar 16, 2018

nicolas-grekas Mar 16, 2018 •

edited

Loading

nicolas-grekas commented Jun 4, 2018

AndreiIgna commented Jun 5, 2018

fabpot Jun 11, 2018

fabpot Jun 11, 2018

AndreiIgna commented Jun 11, 2018

nicolas-grekas commented Jun 19, 2018

Tobion Oct 27, 2018

Tobion Oct 27, 2018

nicolas-grekas commented Nov 1, 2018

[HttpFoundation] Adds getAcceptableFormats() method for Request #26486

[HttpFoundation] Adds getAcceptableFormats() method for Request #26486

Conversation

AndreiIgna commented Mar 11, 2018 • edited Loading

derrabus commented Mar 13, 2018

AndreiIgna commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiIgna commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Jun 4, 2018

AndreiIgna commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiIgna commented Jun 11, 2018

nicolas-grekas commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Nov 1, 2018

AndreiIgna commented Mar 11, 2018 •

edited

Loading

nicolas-grekas Mar 16, 2018 •

edited

Loading