laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

Response::withHeaders() should support a value for "header not present" #1368

Open teo1978 opened 5 years ago

teo1978 commented 5 years ago

Let's say I want to return a response with a bunch of headers:

// In some controller action:
  return response(/* ...*/)->withHeaders([
      'X-foo' => 'foo',
      'X-Bar' => 'bar',
      'X-Whatever' => /* this header may or may not be present */
  ]);

Now, I should be able to pass a value for the header 'X-Whatever' so that the header is not present at all. I would expect that either null or false would serve that purpose. However, neither does. Both produce a header with empty value, i.e. ''.

The obvious usecase for this is the following:

  return response(/* ...*/)->withHeaders([
      'X-foo' => 'foo',
      'X-Bar' => 'bar',
      'X-Whatever' => isSomeConditionTrue()?'somevalue':false // if false, I don't want the header at all
  ]);

That won't work for that purpose, so I have to do the following:

  $headers = [
      'X-foo' => 'foo',
      'X-Bar' => 'bar',
  ]
  if(isSomeConditionTrue()) $headers['X-Whatever'] = 'somevalue';
  return response(/* ... */)->withHeaders($headers);

which is pathetic.

Unfortunately, adding the feature now with either false or null as the conventioanl value for "header not present" would break BC. But I think It's worth it, because any other value would be ugly (what could it be?? A special string? Ugh!), and OTOH the very few times where one could actually want an empty header, they should probably explicitly set it to ''.

I'm astonished that this is not already built-in.

mfn commented 5 years ago

built-in

Would array_filter work for you?

$ php -r '$a=["a" => "b", "c" => "", "d" => null, "e" => false, "f" => true]; var_dump(array_filter($a));'
array(2) {
  ["a"]=>
  string(1) "b"
  ["f"]=>
  bool(true)
}

as in

return response(/* ...*/)->withHeaders(array_filter([
      'X-foo' => 'foo',
      'X-Bar' => 'bar',
      'X-Whatever' => /* this header may or may not be present */
  ]));
teo1978 commented 5 years ago

Would array_filter work for you?

It "works for me" because there are no cases where I actually want an empty header, such as

   X-Actually-Empty-Header:
   X-Foo: Bar

but if I wanted that, I would have no way to explicitly set an empty header and at the same time set a non-existing header in the same array, as array_filter() will filter out '' too.

teo1978 commented 5 years ago

Oh, actually, array_filter() would not work at all, as it wouldn't even allow me to set a header with a value of 0.

mfn commented 5 years ago

Laravel is doing nothing more then delegating to Symfony for setting the headers.

I would expect that either null or false would serve that purpose. However, neither does. Both produce a header with empty value, i.e. ''. … I'm astonished that this is not already built-in.

I wonder if this should be brought upstream, so everyone, not just Laravel, could benefit from your idea.

victorlap commented 5 years ago

If you want to have a header with an empty value, or with 0 as a value, you can of course use the + operator:

return response(/* ...*/)->withHeaders([
      'X-foo' => '',
      'X-Bar' => 0,
] + array_filter([
      'X-Whatever' => /* this header may or may not be present */
]));
$ php -r '$a = ["X-foo" => "", "X-Bar" => 0] + array_filter(["X-Whatever" => false]);var_dump($a);'
array(2) {
  'X-foo' =>
  string(0) ""
  'X-Bar' =>
  int(0)
}
teo1978 commented 5 years ago

Yeah, of course you can achieve that in a number of ways, but isn't that horrible? Usually when something expects an array of key-value pairs and there's a good use case for a key not being present, I expect there to be a way out-of-the-box to specify a "not set" value, unless there's a good reason to (e.g if null, false, '', etc. are all legitimate values for which there is a use). In old Yii 1.1, for example, not sure about headers in particular, but that was a common convention with many methods accepting an associative array as argument.