Posted by sun on May 2, 2009 at 4:05pm
8 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | brianV |
| Status: | closed (fixed) |
| Issue tags: | API clean-up, Needs Documentation |
Issue Summary
#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().
Comments
#1
This patch should do it.
#2
#3
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.
#4
@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?
#5
drupal_add_http_header() works for me.
#6
ok - I will reroll a patch with drupal_add_http_header() in a bit.
#7
New patch
#8
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.
#9
I'd like to get @sun's feedback on the proposed name change.
#10
The last submitted patch failed testing.
#11
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.
#12
Tagging for feature freeze.
#13
New patch rolled, renames the following functions:
drupal_set_headertodrupal_add_http_headerdrupal_get_headertodrupal_get_http_header#14
#15
The last submitted patch failed testing.
#16
Re-rolled. Should come back green.
#17
yay api cleanup!
#19
The last submitted patch failed testing.
#20
/me stops counting how many RTBC patches he needs to re-roll...
#21
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.
#22
FYI: #552478: Make self-closing optional in theme_html_tag() will turn drupal_add_html_head() into a structured array. :)
Reverting status.
#23
Committed to CVS HEAD. Thanks!
#24
Automatically closed -- issue fixed for 2 weeks with no activity.
#25
This change should be mentioned on http://drupal.org/node/224333.
My permission to edit that page seems to have been revoked.
#26
Documentation is critical isn't it!?
#27
#653068: Update documentation is incomplete
#28
Documentation has been added to http://drupal.org/update/modules/6/7.
#29