[HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead #32122

Simperfit · 2019-06-20T20:01:15Z

Q	A
Branch?	4.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	maybe
Tests pass?	yes
Fixed tickets	#31317
License	MIT
Doc PR	todo

the $first param has been deprecated in the get methid
and we are adding a $key parameter to all to get all values from a key as arrays
Do we deprecated the get method ? if so this will be a little bigger in terms of changes.

ro0NL

given now all() vs getAll() ... im wondering if getValue() / getValues() is the better name

overall 👍 for the intent

src/Symfony/Component/HttpFoundation/HeaderBag.php

Simperfit · 2019-06-23T19:27:33Z

Maybe we should introduced the method firstly and then deprecate the get and remove the usage, because like this it would be hard to do both in the same PR. cc @ro0NL

fabpot · 2019-06-27T07:02:48Z

I'm against deprecating get(). It's one a those function used a lot. We can deprecate the second argument and create a new method for getting all values though.

Simperfit · 2019-06-27T07:05:30Z

So getValue becames get and we keep getValues.
I think it's ok too because get is used everywhere in the code base and in project too.

fabpot · 2019-06-27T07:19:40Z

To be honest, I would prefer to not change anything :)

fabpot · 2019-07-08T11:50:15Z

I'd like some other opinion here @symfony/deciders

nicolas-grekas · 2019-07-08T11:58:14Z

I'm for keeping get() and make it always return a string (ie deprecate argument $first) and adding getAll($key): array

lyrixx · 2019-07-08T12:02:24Z

Same opinion as @nicolas-grekas

Simperfit · 2019-07-08T16:20:24Z

PR Updated

src/Symfony/Component/HttpFoundation/HeaderBag.php

Tobion · 2019-07-08T20:04:23Z

I also think we should give set the same treatment as get to be consistent and make it use real types.

create a setValues method
only accept string in set, i.e. trigger a deprecation for anything but string. this would also solve the int problem in [HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead #32122 (comment)

src/Symfony/Component/HttpFoundation/HeaderBag.php

Simperfit · 2019-07-11T05:10:22Z

@Tobion updated

src/Symfony/Component/WebLink/Tests/EventListener/AddLinkHeaderListenerTest.php

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

src/Symfony/Component/HttpKernel/Tests/EventListener/TestSessionListenerTest.php

src/Symfony/Component/WebLink/Tests/EventListener/AddLinkHeaderListenerTest.php

jvasseur · 2019-07-31T06:27:12Z

src/Symfony/Component/HttpFoundation/HeaderBag.php

+        if (1 <= \func_num_args() && null !== $key = func_get_arg(0)) {
+            $key = str_replace('_', '-', strtolower($key));
+
+            return $this->headers[$key] ?? [];


This doesn't solve the problem in the linked issue (the get method having 2 different return types depending on a parameter), now it's all that can return either string[] or string[][] based on the presence of a parameter.

that's fine to me, the proposed interface is a nice and consistent update

src/Symfony/Component/HttpFoundation/HeaderBag.php

nicolas-grekas

I'm adding the missing UPGRADE/CHANGELOG lines.

nicolas-grekas · 2019-08-08T18:15:48Z

Thank you @Simperfit.

…an array and add all($key) instead (Simperfit) This PR was merged into the 4.4 branch. Discussion ---------- [HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | maybe  | Tests pass? | yes  | Fixed tickets | #31317  | License | MIT | Doc PR | todo   the $first param has been deprecated in the get methid and we are adding a $key parameter to all to get all values from a key as arrays Do we deprecated the get method ? if so this will be a little bigger in terms of changes. Commits ------- 2c5a8f1 [HttpFoundation] deprecate using $first in get and added key in all

Tobion · 2019-08-08T19:00:05Z

src/Symfony/Component/HttpFoundation/HeaderBag.php

     * @return array An array of headers
     */
-    public function all()
+    public function all(/*string $key = null*/)


That is a no-go for me. It introduces another of #31317 which this PR tried to fix.
Depending on the arguments it returns an array of arrays or an array of strings.

I know it's the same as on the Messenger Envelope but that was on my todo to fix as was a pain to work with.

carsonbot added Status: Needs Review HttpFoundation Feature labels Jun 20, 2019

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch from e9544ef to fad5ccb Compare June 20, 2019 20:01

ro0NL reviewed Jun 23, 2019

View reviewed changes

Simperfit requested a review from dunglas as a code owner June 23, 2019 13:41

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch 2 times, most recently from 768bfef to 3b7af3a Compare June 23, 2019 14:09

nicolas-grekas added this to the next milestone Jun 23, 2019

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch 3 times, most recently from bf8c439 to 63a4a2d Compare June 27, 2019 06:47

fabpot closed this Jul 8, 2019

fabpot reopened this Jul 8, 2019

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch from 63a4a2d to f8d3ffa Compare July 8, 2019 16:19

Tobion reviewed Jul 8, 2019

View reviewed changes

src/Symfony/Component/HttpFoundation/HeaderBag.php Outdated Show resolved Hide resolved

Tobion reviewed Jul 8, 2019

View reviewed changes

src/Symfony/Component/HttpFoundation/HeaderBag.php Outdated Show resolved Hide resolved

Simperfit changed the title ~~[HttpFoundation] add HeaderBag::getFirst and HeaderBag::getAll~~ [HttpFoundation] deprecating HeaderBag::get returning an array and add HeaderBag::getValues Jul 11, 2019

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch from f8d3ffa to d47783a Compare July 11, 2019 04:11

Simperfit changed the title ~~[HttpFoundation] deprecating HeaderBag::get returning an array and add HeaderBag::getValues~~ [HttpFoundation] deprecating HeaderBag::get/set getting/returning an array and add HeaderBag::getValues/setValues Jul 11, 2019

ro0NL reviewed Jul 31, 2019

View reviewed changes

src/Symfony/Component/WebLink/Tests/EventListener/AddLinkHeaderListenerTest.php Outdated Show resolved Hide resolved

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch 2 times, most recently from aedda78 to 00bdb70 Compare July 31, 2019 05:52

ro0NL reviewed Jul 31, 2019

View reviewed changes

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch from 00bdb70 to d0cc721 Compare July 31, 2019 05:58

jvasseur reviewed Jul 31, 2019

View reviewed changes

nicolas-grekas approved these changes Aug 6, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Aug 6, 2019

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch from d0cc721 to dba2c3f Compare August 6, 2019 11:30

nicolas-grekas changed the title ~~[HttpFoundation] deprecating HeaderBag::get getting an array and add a param to all() to return all values~~ [HttpFoundation] deprecate HeaderBag::get() returning an array and add all($name) instead Aug 6, 2019

nicolas-grekas changed the title ~~[HttpFoundation] deprecate HeaderBag::get() returning an array and add all($name) instead~~ [HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead Aug 6, 2019

lyrixx approved these changes Aug 6, 2019

View reviewed changes

Simperfit force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch 2 times, most recently from c166e23 to d544054 Compare August 6, 2019 12:14

xabbuh reviewed Aug 8, 2019

View reviewed changes

src/Symfony/Component/HttpFoundation/HeaderBag.php Show resolved Hide resolved

[HttpFoundation] deprecate using $first in get and added key in all

2c5a8f1

nicolas-grekas approved these changes Aug 8, 2019

View reviewed changes

nicolas-grekas force-pushed the feature/add-getFirst-and-getAll-to-not-do-both-on-get branch from d544054 to 2c5a8f1 Compare August 8, 2019 18:15

nicolas-grekas mentioned this pull request Aug 8, 2019

[HttpFoundation] deprecate HeaderBag::get() returning an array and add … symfony/symfony-docs#12122

Closed

nicolas-grekas merged commit 2c5a8f1 into symfony:4.4 Aug 8, 2019

Tobion reviewed Aug 8, 2019

View reviewed changes

Tobion mentioned this pull request Aug 8, 2019

Statically Analysable HeaderBag Get Methods #31317

Closed

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

This was referenced Nov 12, 2019

Release v4.4.0-BETA1 #34338

Merged

Release v5.0.0-BETA1 #34339

Merged

Tobion mentioned this pull request Feb 5, 2020

[HttpFoundation] Add InputBag #34363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead #32122

[HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead #32122

Simperfit commented Jun 20, 2019 •

edited

Loading

ro0NL left a comment

Simperfit commented Jun 23, 2019

fabpot commented Jun 27, 2019

Simperfit commented Jun 27, 2019

fabpot commented Jun 27, 2019

fabpot commented Jul 8, 2019

nicolas-grekas commented Jul 8, 2019 •

edited

Loading

lyrixx commented Jul 8, 2019

Simperfit commented Jul 8, 2019

Tobion commented Jul 8, 2019

Simperfit commented Jul 11, 2019

jvasseur Jul 31, 2019

nicolas-grekas Aug 6, 2019 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas commented Aug 8, 2019

Tobion Aug 8, 2019

Tobion Aug 8, 2019

[HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead #32122

[HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead #32122

Conversation

Simperfit commented Jun 20, 2019 • edited Loading

ro0NL left a comment

Choose a reason for hiding this comment

Simperfit commented Jun 23, 2019

fabpot commented Jun 27, 2019

Simperfit commented Jun 27, 2019

fabpot commented Jun 27, 2019

fabpot commented Jul 8, 2019

nicolas-grekas commented Jul 8, 2019 • edited Loading

lyrixx commented Jul 8, 2019

Simperfit commented Jul 8, 2019

Tobion commented Jul 8, 2019

Simperfit commented Jul 11, 2019

jvasseur Jul 31, 2019

Choose a reason for hiding this comment

nicolas-grekas Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 8, 2019

Tobion Aug 8, 2019

Choose a reason for hiding this comment

Tobion Aug 8, 2019

Choose a reason for hiding this comment

Simperfit commented Jun 20, 2019 •

edited

Loading

nicolas-grekas commented Jul 8, 2019 •

edited

Loading

nicolas-grekas Aug 6, 2019 •

edited

Loading