In drupal 6, when you wanted to add a header it was:
drupal_set_header($header);

Now, its replacement as of
#451604: Rename drupal_set_header()

function drupal_add_http_header($name = NULL, $value = NULL, $append = FALSE) {
  // The headers as name/value pairs.
  $headers = &drupal_static(__FUNCTION__, array());

  if (!isset($name)) {
    return $headers;
  }

Firstly, I hate this pattern. PHP doesn't provide overloading, and this is a terrible thing drupal does ALL OVER THE PLACE. Why do we need to have drupal_add_js() RETURN a list of things which have been added... this is illogical. drupal_get_js() or drupal_get_added_js() maybe. This is where OOP design is generally implemented (holding state inside an encapsulated storage mechanism). We could do better even without building private vars and accessors / mutators.

Anyway, I digress. This pissed me off today after I finally realized that the API had changed. That it changes is fine. In fact, I think it is much better. BUT it should at least fail brutally if you try to use it in the way it USED to work.

This patch does that by adding this:

if (isset($value) && empty($name)) {
    throw new Exception('In Drupal 7, headers are passed in as key:value pairs.  For example: $headers = array("Content-type" => "application/octet-stream").');
  }

Now, if someone uses file_transfer() during a D6 to D7 upgrade, they won't be pleasantly surprised to find their code just silently fails.

file_transfer($uri, array('Content-type: audio/mp3'));

Before this patch:

No affect on the download, and no warning.

After the patch:
Big 'ol exception.

CommentFileSizeAuthor
#9 732486-drupal-add-http-header-wtf.patch8.3 KBdamien tournoud
#4 732486-drupal-add-http-header-wtf.patch8.3 KBdamien tournoud
#3 732486-drupal-add-http-header-wtf.patch8.1 KBdamien tournoud
header_bc_error_wtf.patch593 bytesJacobSingh

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Are you suggesting introducing a pattern where we'd add code to warn people who still keep using the old APIs (given your general opposition to how this pattern works) or just in this special case?

JacobSingh’s picture

JacobSingh commented

I think that anytime a function signature does not change (in terms of the # of arguments or names of arguments) and still executes without a warning or error but produces totally different results it is a bug. Some should through exceptions, some should throw warnings, but the user should know that it used to work, and now it is failing, and here is why.

In this particular case, I find the pattern of a function containing the verb "add" which is actually for a "get" when you don't pass certain magic arguments is a travesty of justice. I know this is all over Drupal, but I've never liked it and it would be nice to un-wtf this in D8.

Also, besides being an upgrade WTF as it is, if you pass in a value but no name, it should bail because that is an error. Returning an array when I intended to add a header is not correct IMO and an exception needs to be thrown.

Best,
J

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 732486-drupal-add-http-header-wtf.patch8.1 KB

Well, let's fix the function signature, instead of introducing more cruft in the code.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 732486-drupal-add-http-header-wtf.patch8.3 KB

Forgot to remove dead code in drupal_add_http_header().

From the patch:

@@ -2722,7 +2722,7 @@ function _db_error_page($error = '') {
   global $db_type;
   drupal_language_initialize();
   drupal_maintenance_theme();
-  drupal_add_http_header($_SERVER['SERVER_PROTOCOL'] . ' 503 Service Unavailable');
+  drupal_add_http_header('Status', '503 Service Unavailable');
   drupal_set_title('Site offline');
 }

That was actually a critical bug: database error pages are probably served using a 200 header right now.

JacobSingh’s picture

JacobSingh commented

Should we also remove this then?

If we don't, we get the same WTF I got during a D6 -> D7 upgrade using file_transfer() (except perhaps with a warning this time).

 if (!isset($name)) {
     return $headers;
   }

Best,
Jacob

damien tournoud’s picture

damien tournoud commented

@JacobSingh: yes, that was an oversight, already removed in the last patch.

JacobSingh’s picture

JacobSingh commented

Looks perfect.

As soon as the bot comes back I give it to big thumbs up :)

eric_a’s picture

eric_a commented 
-  if (!isset($value)) {
+  if ($value == FALSE) {
     $headers[$name_lower] = FALSE;
   }

This patch gives us now many ways to unset a header, at the same time denying us the use of '0' and '' as header field values. Strict comparison, then?

-    if ($name_lower == ':status') {
+    if ($name_lower == 'status') {
       header($_SERVER['SERVER_PROTOCOL'] . ' ' . $value);
     }

So :status/:status gets replaced with [Status, any casing]/status. Is there any special reason to pick these over for example Status/:status or :status/:status? I haven't seen the issue that introduced :status, but I can imagine that it was picked to make it very, very unlikely to ever clash with any header.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 732486-drupal-add-http-header-wtf.patch8.3 KB

That comparison should have been strict. Fixed.

Status is actually a normalized header (normalized by CGI/1.1). It's the perfect choice here, and risks no collision with an existing header. When drupal_send_headers() will set header($_SERVER['SERVER_PROTOCOL'] . ' ' . $value), PHP will output either a direct HTTP status header (if run with mod_php) or a Status: ... header, if run with cgi/fastcgi.

dries’s picture

dries commented

I agree with this change. Much better.

moshe weitzman’s picture

moshe weitzman commented
Title: Developer WTF and anti-pattern in drupal_add_http_header » drupal_add_http_header signature improvement

remove opinion in title.

JacobSingh’s picture

JacobSingh commented
Title: drupal_add_http_header signature improvement » drupal_add_http_header $name req ; make Status a normal header and drupal_add_http header shouldn't return a list of headers.
Status: Needs review » Reviewed & tested by the community

Sorry, I was ranting...

Anyway, the new title isn't exactly accurate, but obviously closer than the original :)

I've changed it to more accurately reflect the change.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

eric_a’s picture

eric_a commented
Status: Fixed » Needs work

Update docs... http://drupal.org/update/modules/6/7#http_header_functions

Drupal 7.x:

drupal_add_http_header('500 Internal server error');
eric_a’s picture

eric_a commented
Status: Needs work » Fixed

Added link to this issue and fixed example (Added the special 'Status' header name). http://drupal.org/update/modules/6/7#http_header_functions

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

This change was not included in the chronological list of API changes on the http://drupal.org/update/modules/6/7 page, added.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.