Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Adds getAcceptableFormats() method for Request#26486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[HttpFoundation] Adds getAcceptableFormats() method for Request#26486
Uh oh!
There was an error while loading.Please reload this page.
Conversation
derrabus commentedMar 13, 2018
It looks like this PR contains unrelated commits. Could you do a rebase against master? |
08a9337 to00acc8cCompareAndreiIgna commentedMar 13, 2018
Oops used GitHub desktop, which did something unexpected to the rebase. Should be ok now |
| protected$format; | ||
| /** | ||
| * @var string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should bearray.
| * 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should beadded to be consistent with the wording of other entries in that file.
AndreiIgna commentedMar 15, 2018
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 this is useful and included in next release, I'll make the addition in documentation repo as well |
| return$this->acceptableFormats; | ||
| } | ||
| return$this->acceptableFormats =array_values(array_unique(array_filter(array_map(array($this,'getFormat'),$this->getAcceptableContentTypes())))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
isarray_values actually required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is added because the mix ofarray_filter andarray_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
| * | ||
| * @return array List of acceptable formats by the client | ||
| */ | ||
| publicfunctiongetAcceptableFormats() |
nicolas-grekasMar 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing return type (and removal of the corresponding docblock line since it will become useless)
1e9bf3f toeef572bComparenicolas-grekas commentedJun 4, 2018
@AndreiIgna would you mind rebasing and moving the line in the changelog on 4.2? |
eef572b to7f062b1CompareAndreiIgna commentedJun 5, 2018
Yes sure, is updated now |
| /** | ||
| * @var array | ||
| */ | ||
| protected$acceptableFormats; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be private
| 4.2.0 | ||
| ----- | ||
| * Adds`getAcceptableFormats()` for reading acceptable formats based on Accept header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
added
7f062b1 to9bfb693CompareAndreiIgna commentedJun 11, 2018
Updated based on last review. While checking in more detail the similar |
9bfb693 to8a127eaComparenicolas-grekas commentedJun 19, 2018
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#... <!-- required for new features -->Adds a new method `getAcceptableFormats()` for `Request`. This reads the content types in Accept header and maps them to formats already defined in `Request` classIn 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 documentationCommits-------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() methodAdds 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`refsymfony/symfony#26486Commits-------65d100e [HttpFoundation] Add info for getAcceptableFormats() method
| $request =newRequest(); | ||
| $request->headers->set('Accept','text/html, application/xhtml+xml, application/xml;q=0.9, */*'); | ||
| $this->assertEquals(array('html','xml'),$request->getAcceptableFormats()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedNov 1, 2018
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 commit8a127ea.| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#26486| License | MIT| Doc PR |As I said in#26486 (comment) and people wonder inhttps://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"
Uh oh!
There was an error while loading.Please reload this page.
Adds a new method
getAcceptableFormats()forRequest. This reads the content types in Accept header and maps them to formats already defined inRequestclassIn a request made by a browser, based on the default Accept header, the
getAcceptableFormats()will return an array['html', 'xml']In progress