#372563: Rename drupal_set_html_head() renamed drupal_set_html_head() to drupal_add_html_head().

We still have drupal_set_header(), which is totally confusing now. One uses 'add', the other 'set'.

Let's rename drupal_set_header() equally to drupal_add_header().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brianV’s picture

Status: Active » Needs review
FileSize
15.41 KB

This patch should do it.

brianV’s picture

Assigned: Unassigned » brianV
Dries’s picture

Priority: Critical » Normal

I'm not sure it is confusing. What is confusing is that you can't tell weather it is a HTTP header or a HTML head(er). Certainly not critical.

brianV’s picture

@Dries,

'set' and 'add' both work equally well for this function. 'add' was suggested to match the other function.

Perhaps 'drupal_set_http_header()' or 'drupal_add_http_header()' would be more apt?

Dries’s picture

drupal_add_http_header() works for me.

brianV’s picture

ok - I will reroll a patch with drupal_add_http_header() in a bit.

brianV’s picture

New patch

c960657’s picture

Note that drupal_set_header() not only adds new headers, but also allows replacing and unsetting headers previously set. drupal_add_http_header() might still be a better name, though.

Dries’s picture

I'd like to get @sun's feedback on the proposed name change.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

drupal_add_http_header() would work for me as well.

Though - one could also argue that we have

- drupal_chmod() as replacement for PHP's chmod()
- drupal_function_exists() as replacement for PHP's function_exists()
- drupal_mail() as replacement for mail()
- and many more...

So, a different alternative would be to just rename it to drupal_header() - as replacement for PHP's header().

Please also note that in case we rename to drupal_add_http_header(), we should probably also rename drupal_get_header() accordingly.

sun’s picture

Issue tags: +API clean-up

Tagging for feature freeze.

brianV’s picture

New patch rolled, renames the following functions:

  • drupal_set_header to drupal_add_http_header
  • drupal_get_header to drupal_get_http_header
brianV’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
19.47 KB

Re-rolled. Should come back green.

chx’s picture

Status: Needs review » Reviewed & tested by the community

yay api cleanup!

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
19.73 KB

/me stops counting how many RTBC patches he needs to re-roll...

c960657’s picture

I like the addition of "_http_" to the function name.

I'm not sure that I like the change from "_set_" to "_add_", though. drupal_add_html_head() simply adds the supplied string to an unstructured variable. The added values cannot be read or changed later on. The drupal_get_header()/drupal_set_header() pair, on the other hand, reads and writes named entries in an array. I think this behaviour is closer related to cache_get()/cache_set() than to drupal_add_html_head().

However, if we decide to stick with the proposed function names, the patch looks RTBC to me.

sun’s picture

Status: Needs review » Reviewed & tested by the community

FYI: #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag will turn drupal_add_html_head() into a structured array. :)

Reverting status.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

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

c960657’s picture

Status: Closed (fixed) » Needs work
Issue tags: +Needs documentation

This change should be mentioned on http://drupal.org/node/224333.

My permission to edit that page seems to have been revoked.

casey’s picture

Priority: Normal » Critical

Documentation is critical isn't it!?

catch’s picture

Priority: Critical » Normal
brianV’s picture

Documentation has been added to http://drupal.org/update/modules/6/7.

brianV’s picture

Status: Needs work » Closed (fixed)