Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag

This has nothing to do with Shorten URLs--this module just retrieves the shortened URL. It's up to other modules which use the shortened URL to designate that they do so. In particular, your request may be more relevant to the ShortURL module, because it doesn't require a third-party service to come up with a shortened URL. In addition, for Shorten URLs to do what you're suggesting could dramatically slow down all page loads, not to mention that the shortlinks could change every time a page was loaded on some systems.

Also, the shortlink "standard" is not quite a standard [yet] as it has to compete with shorturl and canonical.

Had an interesting talk at lunch with samj about short url and cannonical url in core - moving the issue

Core could provide the the node/$nid link by default as a short link, and should also provide a rel=canonical link to help with the SEO problem of duplicate page content, especially if path module is enabled.

http://googlewebmastercentral.blogspot.com/2009/02/specify-your-canonica...

I think this is contrib territory. IceCream is right, http://drupal.org/project/shorturl is what you're looking for. Also check http://drupal.org/project/shortlink .

I think this could go in core eventually, but not until some kind of real standard has been solidified. For now, it's fine in contrib.

discussed with Rob last night - I think basic support for this in core AND a better API for adding links, etc, to the page header would be very useful.

We are already adding rel="canonical" in comment module, so really we jsut want to improve on that existing code

this issue also describes part of what we are doing #567482: Allow <link> tags to be added as attached structures

patch written 50% by samj and by me at code sprint.

this changes the API. Function signature for drupal_add_html_head() changes substantially, and a new optional param for drupal_add_link(). Also adds a new system element and theme function.

needed an extra space in the theme function to match the expected test strings, also added some suggestions for the changelog

left a bit of stray code in system.module

Also fix doxygen for function drupal_add_html_head()

as a side note, this feature is already deployed on wordpress.com (including cnn.com blogs apparently) - example headers:

HTTP/1.1 200 OK
Server: nginx
Date: Sat, 05 Sep 2009 20:14:25 GMT
Content-Type: text/html; charset=UTF-8
Connection: close
Last-Modified: Sat, 05 Sep 2009 20:14:00 +0000
Cache-Control: max-age=275, must-revalidate
Vary: Cookie
X-hacker: If you're reading this, you should visit automattic.com/jobs and apply to join the fun, mention this header.
X-Pingback: http ://pagingdrgupta.blogs.cnn.com/xmlrpc.php
Link: <http://wp.me/pcFQ9-oL>; rel=shortlink

Now the #attached patch is in, should this be using that? Looks like a good change.

+++ includes/common.inc
@@ -250,12 +250,24 @@ function drupal_get_rdf_namespaces() {
+    if (isset($key)) {
+      $stored_head[$key] = $data;
+    } else {
+      $stored_head[] = $data;
+    }

(minor) Coding style issue.

+++ includes/common.inc
@@ -2503,9 +2529,44 @@ function base_path() {
+  $head_element = array(
+	'#type' => 'head_element',
+    '#value' => 'link',
+	'#attributes' => $attributes,
+  );

(minor) Wrong indentation.

+++ modules/system/system.module
@@ -3022,6 +3029,14 @@ function theme_system_compact_link() {
+/**
+ * Send Drupal and the major version number in the HTTP headers.
+ *
+ * @ingroup themeable
+ */
+function theme_head_element(array $element) {
+  return '<' . $element['#value'] . drupal_attributes($element['#attributes']) ." />\n";
+}

This should be moved to theme.inc

+++ includes/common.inc
@@ -250,12 +250,24 @@ function drupal_get_rdf_namespaces() {
+ * @param $data
+ *   Optional renderable array, of type 'head_element' or 'head_title'.

Can't find any other reference to 'head_title' in the patch.

+++ modules/system/system.module
@@ -486,6 +489,10 @@ function system_elements() {
+  $type['head_element'] = array(
+    '#theme' => 'head_element',
+  );
+
@@ -3029,7 +3044,14 @@ function theme_system_compact_link() {
+  drupal_add_html_head(array(

We don't add the '_element' suffix to elements. We should name it 'html_head', to be consistent with drupal_add_html_head()

+++ modules/system/system.module
@@ -3029,7 +3044,14 @@ function theme_system_compact_link() {
 function theme_meta_generator_html($version = VERSION) {
-  drupal_add_html_head('<meta name="Generator" content="Drupal ' . $version . ' (http://drupal.org)" />');
+  drupal_add_html_head(array(
+    '#type' => 'head_element',
+    '#value' => 'meta',
+    '#attributes' => array(
+      'name' => 'Generator',
+      'content' => 'Drupal ' . $version . ' (http://drupal.org)',
+    ),
+  ));
 }

If we add hook_html_head_alter(), we don't need a theme function for this. We should use the $key param of drupal_add_html_head() to allow modules to alter the META Generator.

If we want to allow <link> and <meta> tags to be attached to render() structures, then we should keep drupal_add_html_head() as an API function, which receives an array options. Then, we should work on drupal_get_html_head() to generate a render() structure with all the data added through drupal_add_html_head(), allow modues to alter, render it, and finally return the rendered output, ready to be put in <head>. The attached patch implements it this way.

Changes from patch #11:
- The new element introduced in system_elements() is named head_tag.
- The theme function to generate the tag to be added to the head section looks like this:

function theme_head_tag(array $element) {
  return '<' . $element['#tag'] . drupal_attributes($element['#attributes']) ." />\n";
}

So, the property used to specify the tag is #tag and not #value.
- The theme function was moved to theme.inc
- Changes in drupal_get_html_head() to generate the render() structure there and return the rendered output.
- Removed the $header parameter in drupal_add_link(). This option should be passed in one of the keys of $attributes array. Easier to attach links to render() structures. See changes in openid_test.module.

New changes added:
- Removed meta_generator_html and meta_generator_header theme hooks as they are not required any more. If we add hook_html_head_alter(), we don't need a theme function for this. We should use the $key param of drupal_add_html_head() to allow modules to alter the META Generator.
- Added new function drupal_html_head_defaults(). Returns an array of head tag elements added by default to all pages generated by Drupal.
- Moved hardcoded content type and Generator <meta> tags to drupal_html_head_defaults().

- Fixed parameters in batch.inc

@dropcube - why #tag? I don't see why that's more meaningful. We already use #value for rendering, so it seems a familiar attribute. If we want it to be more meaningful, it should be something like #element_type

I really don't like the change to drupal_add_html_head() - burying the parameters in another layer of array doesn't seem very useful.

re: 'head_title', I was thinking of adding that so we cold get the title in too, but seems like more in scope for a follow-up patch since it would touch lots of tpl files.

We are using "element" in too totally separate contexts here. that's why 'head_element' as a TYPE makes sense - it renders to a single XHTML element in the document.

Also, we don't need a defaults function since we are, essentially, already doing this in drupal_get_html_head().

- Fixed openid tests.

I'm working on a re-roll. Merging in some of the suggested changes above (like removing the meta theme functions), plus I wrote alter hook api doc on the plane.

@pwolanin:

We already use #value for rendering, so it seems a familiar attribute. If we want it to be more meaningful, it should be something like #element_type

We use #markup in the markup element type, and #value for elements that have a 'value'. Here we are adding tags to the XHTML <head> section, so the element name 'head_tag' and the property '#tag' sounds fine to me...

I really don't like the change to drupal_add_html_head() - burying the parameters in another layer of array doesn't seem very useful.

The workflow I proposed is as follow:
- drupal_add_html_head() is used to add data to head, as an API function, or as a callback of #attached html head data.
- when the html head is rendered, then get the data from drupal_add_html_head()
- generate a render() structure.
- allow modules to alter it
- render the structure
- at this point return the rendered output, ready to be put in head.

I don't see why we should pass an element as the parameter, it's more complicated to use it to attach html_head data to other elements.

Also, we don't need a defaults function since we are, essentially, already doing this in drupal_get_html_head().

Just to have a function where the default html head tags are added. For clarity and readability of the code.

many types use #value: http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....

Used by: button, hidden, image_button, item, markup, submit, token, value

however, perhaps this is a better example to follow:

  $type['list'] = array(
    '#title' => '',
    '#list_type' => 'ul',
    '#attributes' => array(),
    '#items' => array(),
  );

at least #tag_type would be a little more explicit that just #tag

@dropcube - sorry I missed you in IRC - was having dinner

I think it's easiest to pass a full element in to drupal_add_html_head since that we we can set any #attached we want, etc.

Also, as above, I think later we could consider adding a separate element type for the title.

This patch merges in several of the above suggestions, but is a continuation from my last patch.

fixed a bug with the HTTP headers, remove a duplicate function call in system.module, improved comments, etc.

This is just cleanup / condolidation on the existing patch now, but all looks fine.

I reviewed this last night but didn't have time to comment, so Peter asked me to review this one last time this morning.

We discussed a number of things in IRC but the bottom line was this looks WAY more "drupaly" to my eyes, which I think is a big win for people who are already used to menu and fapi. So from that perspective I'm very much liking where this has gone. Also, the addition of canonical links into drupal is (on a personal note) a really big win so, ++ all around here.

Eclipse

While I applaud the valiant effort that went into this patch at the very last possible minute ;), I see issues here bigger than mere coding standards type stuff, so I'm not sure this really qualifies as RTBC. Other than dropcube and the patch authors, I don't see anyone else really taking a deep-dive into its guts. It feels like this is going to require another 2-3 go-arounds to be truly RTBC and I believe that exempts it from code slush. I will need to talk it over with Dries.

Here's what I found in my initial review, which is probably not exhaustive since I'm fighting a righteous headache today. It would also be nice to get a chime-in here from Moshe to discuss how it fits in with render API; I believe it's consistent but it'd be nice to have confirmation.

Additionally, I'm a bit nervous about supporting rel=shortlink given that Drupal 7 is going to be around until at least 2013, and this "specification" as it were a) is in some wiki page somewhere, and b) appears to be in competition with other similar specifications. rel=canonical at least had support of the major search engines, even if not the W3C. This one seems a bit fuzzier. It seems like if we used this patch to turn <head> into a renderable array, this would be easy to facilitate from contrib, which is probably where it belongs. I realize WordPress.com implements it, but they have quite a bit more flexibility to change their minds on a whim about what specifications they offer as part of their "core" package than Drupal 7 does, since Drupal 7 is a downloadable product, not a hosted one.

+++ includes/batch.inc
@@ -190,7 +190,14 @@ function _batch_progress_page_nojs() {
-  drupal_add_html_head('<meta http-equiv="Refresh" content="0; URL=' . $url . '">');
+  drupal_add_html_head(array(
+    '#type' => 'head_tag',
+    '#tag_type' => 'meta',
+    '#attributes' => array(
+      'http-equiv' => 'Refresh',
+      'content' => '0; URL=' . $url,
+    ),
+  ));

This just looks completely weird to me.

1. #type => "head_tag"? A head tag is <head> in my lingo, which is not what this is at all. This is adding a meta tag within the document's header.

2. What's wrong with #type => markup since that's what this is, and then we're not forcing people to learn yet another renderable type?

3. There is an amazing array of properties required to describe this when in my alter hook, realistically the only thing I'm going to change is one of the attributes of the #attributes sub-properties. So that's roughly a 5:1 ratio between "meta data" (pardon the pun) and "interesting" data. Seems funny to me.

Is it possible to compact the syntax of this? maybe passing #attributes to #type => markup? not sure...

+++ includes/common.inc
@@ -250,12 +250,25 @@ function drupal_get_rdf_namespaces() {
+ * @param $key
+ *   Optional key for identifying the element in hook_html_head_alter().

Why would this be optional? Everywhere else I have to specify an element ID. We should do that here as well.

+++ includes/common.inc
@@ -2503,9 +2542,47 @@ function base_path() {
+function drupal_add_link($attributes, $header = FALSE) {

Why do we need two functions for doing the same thing?

+++ includes/common.inc
@@ -2503,9 +2542,47 @@ function base_path() {
+  // Mimic drupal_attributes() but with different seperators.

:(

+++ includes/theme.inc
@@ -1843,6 +1843,15 @@ function theme_feed_icon($url, $title) {
+ * Generate the output for an xhtml element in the head region of the page.

Here and elsewhere, xhtml should be XHTML, and head (or whatever tag) should be <head>

ok, well we can make a much more conceptually simple patch if we are willing to forgo having a new element type and the attributes array for the alter hook and just use #markup. By adding an optional param to drupal_attributes we also avoid code duplication, and by requiring a key we at least provide an easy handle for the alter hook.

note that an important reason for passing a full element into drupal_add_html_head() is that we can use #attached, or a module with more complex needs could define an element type and theme function.

missed getting $separator into drupal_attributes() actual implode()

An alternative here would be to do some of this via a "theme" function like the meta-generated stuff is no, but frankly I think that's an abominable abuse of the theme system that should not have been committed.

Discussed with chx - he likes the approach of structured data better, and sees that as obviating the need to supply a key.

He also suggested we need some tests for this (though openID and batch already tests some of it).

After looking at the code more, I also went back to what dropcube suggests of just '#tab' and made the theme function generic for any html tag.

oops - left #tag_type in browser_test module

+++ includes/common.inc
@@ -264,8 +274,34 @@ function drupal_add_html_head($data = NULL) {
+  // Make sure the Content-Type comes first.
+  $head_elements = array_merge($meta_elements, $head_elements);

This seems confusing to me, note that $head_elements may contain <meta> elements as well. As I suggested in other comment, would be great to have a separated helper function where the *default* elements are added, and each meta tag well documented.

+++ includes/common.inc
@@ -2503,9 +2542,28 @@ function base_path() {
+  // Add a LINK element under the xhtml HEAD element.

(minor) As commented by webchick, here and elsewhere, xhtml should be XHTML, and head (or whatever tag) should be .

This review is powered by Dreditor.

ok, minor re-roll per drupcube's suggestions.

I believe that this is a great patch. Here is a point for discussion:

+++ includes/common.inc
@@ -2300,17 +2343,20 @@ function url($path = NULL, array $options = array()) {
-function drupal_attributes(array $attributes = array()) {
+function drupal_attributes(array $attributes = array(), $separator = ' ') {

drupal_attributes() is defined as returning "An HTML string ready for insertion in a tag". By adding a separator argument, I believe we are bending the definition a little bit too much.

+++ includes/common.inc
@@ -2500,12 +2546,28 @@ function base_path() {
+  if ($header) {
+    // Set a HTTP Link: header also.
+    $href = '<' . check_plain($attributes['href']) . '>';
+    unset($attributes['href']);
+    $element['#attached']['drupal_set_header'][] = array('Link',  $href . drupal_attributes($attributes, '; '), TRUE);
+  }

In addition, I don't believe that those HTTP arguments should be HTML-encoded.

This review is powered by Dreditor.

ok, well DamZ and Heine agree that we should not apply check_plain to HTTP header strings (which makes sense since HTML entities don't have a special meaning in HTTP) and in fact a Link: header may include a anchor attribute with < >

So, this adds a separate function for header attributes that similar to drupal_attributes()

Would removing the automatic check_plain() from drupal_attributes() remove the need for that extra function? #522716: drupal_attributes() calls check_plain() on all attributes indiscriminately Most of the time we're check_plain()-ing module-defind strings like 'active'.

No, since we still need a '; ' separator instead of ' '.

Recording my feedback which was delivered via IRC a couple days ago ...

I'm pleased that folks are starting to use drupal_render as their first choice for building stuff. Thanks.

But I think that HEAD building code is a bad match for drupal_render() at this time. We are using drupal_render() all over the place to build up $page and then call drupal_render($page). Thats lovely. But this code runs after that - it runs during template_page_preprocess() when we need to build up the HEAD HTML so we decided to build a render structure and then immediately render it. Thats unnecessarily complex, IMO.

I would prefer if we just built up an array of HEAD items and let modules alter them in right there. drupal_get_css() is an example of this pattern. This patch has an added complication is in that it wants to set an http header. But we ought to find a simpler solution for that.

I'm not strongly opposed to this though. If it goes in, I won't weep. I've said my peace.

I think this is now following a similar pattern to drupal_add_css() and drupal_get_css(), except that it's harder auto-determine a meaningful key (with CSS it's the file name) so either we retain some structured data as in the patch, or we have to do string parsing in the _alter hook to find what we're after.

Also, we have no _alter capability for HTTP headers, so the approach here at least exposes the data before header() is called.

Like theme_links(), I think it i reasonable for callers to provide a key when they are adding items to HEAD.

About the http header - maybe in the process phase (a recently added phase after preprocess), we check the HEAD array and if the shortlink key is still present, we add the http header and implode the array to a string.

There is still debate ongoing about a new html template that would include the head region. It seems like that will get committed in some form, at which point this moves out of the page realm and into being part of the structured data we can feed to that theme function/template.

@moshe - I'd be happy to have the callers provide a key, though in the current approach there shoudl be enough information without it. The patch currently uses this attached method to set 3 headers, and provide a easier way to generally add Link: headers. I don't think we want to special case any particular header.

While reviewing this patch, I made the following minor changes:
- the doxygen for drupal_add_html_head() was a bit messy. Fixed that in the attached patch.
- also improved the spacing of the doxygen of drupal_header_attributes(), hook_html_head_alter()

The code makes perfect sense — I understood 95% of the changes after the first read and 100% after the second read. I think that after the theme/drupal_render() stuff is worked out, this is good to go.

testbot seems hung up on re-testing, so here's a re-roll of Wim's patch with no changes

ok, minor modification to add back the key per moshe. To keep drupal_add_link() simple, it assumes that any REL is unique.

These two changes make life easier for themers/developers, since they can, for example, override the core meta generator element by just adding another tag with the same key.

Overall this provides a lot of contextual improvements, and some more API firepower and I like it. I didn't run this code, but I assume it works given the issue queue and the people who wrote it.

What follows are nit-picky style and documentation things which are "take it or leave it"

$head_elements = array_merge(_drupal_default_html_head(), drupal_add_html_head());

Wouldn't it be better to actually call drupal_add_html_head() inside of _drupal_default_html_head() so that we unify the API?

+ * @param $data
+ *   Optional renderable array. If #type is not set then 'html_tag' will be
+ *   added as a default #type.

While I'm aware Drupal is full of them, I don't like APIs that take "overloaded" arguments like this. Isn't it possible to just change every other call to drupal_add_html_head to use the new signature (an array)

I don't think this will cause a lot of module upgrade pain, because few modules use it.

function drupal_header_attributes

Should this be prefixed with a "_" ? or otherwise marked as a utility function? Perhaps use a "get" or "format" in the name?
Maybe drupal_format_header_attributes()
or drupal_header_attributes_to_string()

html_tag used everywhere

Totally nitpicky, but shouldn't this be xml or xhtml or something? There isn't anything HTMLy about it. Is tag even the right noun? I'm honestly not sure if it should be element or tag...

K, that's it. Nice one!

The array_merge() is important since it makes sure that those elements come first in the final listing and also that a user-added element with the same key overwrites the default.

I guess the alternative would be to add these as the first elements to the array in drupal_add_html_head() the first time it's called.

The $data param is only optional when you are calling the function in order to just get the saved elements - that pattern is already used many places in core, though in some cases there is an extra wrapper function.

re: 'tag' vs. 'element' - I started using element in my initial patch, but based on the suggestion from dropcube and looking at the usage in the rest of core, 'tag' is more consistent with other usage and avoids confusion between a 'renderable element' (i.e. what you pass to drupal_render()) vs. a xhtml element (a.k.a tag).

I'm not sure I agree that adding rel=shortlink is a good idea. As stated by webchick, the specification is still very young.

Also, compared to WordPress.com that changes e.g. http://richardwiseman.wordpress.com/2009/09/18/its-the-friday-puzzle-25/ to http://wp.me/ppky2-tS, Drupal doesn't make most URLs much shorter as long as the short URL still uses the same hostname (unless you have a habit of specifying very long path aliases).

If we forget Twitter and friends for a moment (because their 140-character limit is (mostly) self-imposed and (mostly) a clever way of branding the service), are shortlinks really that relevant?

People using pathauto do tend to have quite long path names - e.g.:

compare:
http://crackingdrupal.com/blog/greggles/easier-and-safer-drupal-developm...

to:
http://crackingdrupal.com/node/24

The point of providing a shortlink is so that YOU (as site owner) have some hope of controlling which link or service is used to refer to your site.
I think this would be a nice feature to have out of the box, but the API changes are more important in the end.

Re-arrange the code a little in response to Jacob, and use 'xhtml_tag' instead of 'html_tag'

fix typo

very minor conflict - re-rolled.

Neaty. :)

Note: I did not read *any* of the follow-ups above and reviewed the latest patch only. If any question of mine happens to be already explained in the issue, then it needs to be explained in the patch instead.

+++ CHANGELOG.txt
@@ -142,6 +142,13 @@ Drupal 7.0, xxxx-xx-xx (development version)
 - Added RDF support:
     * Modules can declare RDF namespaces which are serialized in the <html> tag
       for RDFa support.
+- Search engine optimization and web linking:
+     * Added a rel="canonical" link on node and comment pages to prevent
+       duplicate content indexing by search engines.

Minor: Wrong indentation here.

+++ includes/batch.inc
@@ -190,7 +190,14 @@ function _batch_progress_page_nojs() {
+    '#tag' => 'meta',
+++ includes/theme.inc
@@ -1849,6 +1849,20 @@ function theme_feed_icon($url, $title) {
 /**
+ * Generate the output for a generic XHTML tag with attributes.
+ *
+ * @ingroup themeable
+ */
+function theme_xhtml_tag(array $element) {
+  if (!isset($element['#value'])) {
+    return '<' . $element['#tag'] . drupal_attributes($element['#attributes']) . " />\n";

"#tag" ? Isn't this a DOM element? I.e. "#element" ?

+++ includes/batch.inc
@@ -190,7 +190,14 @@ function _batch_progress_page_nojs() {
+  drupal_add_html_head($element, 'batch_progress_meta_refresh');
+++ includes/common.inc
@@ -250,22 +250,71 @@ function drupal_get_rdf_namespaces() {
+ * @param $key
+ *   A unique string key identifying the data.  Required if $data is not NULL.
...
+function drupal_add_html_head($data = NULL, $key = NULL) {

Sorry, but I don't understand the second argument. This needs much more PHPDoc (probably not in @param but in the general PHPDoc description for drupal_add_html_head()) to make me understand.

I see the usage in drupal_add_link(), so that at least deserves a @see - though I still don't get it. :-/

+++ includes/common.inc
@@ -250,22 +250,71 @@ function drupal_get_rdf_namespaces() {
+function _drupal_default_html_head() {
+  // Add default elements. Make sure the Content-Type comes first because the
+  // IE browser may be vulnerable to XSS via encoding attacks from any content
+  // that comes before this META tag, such as a TITLE tag.
+  $elements['system_meta_content_type'] = array(

How about #weight then?

+++ includes/common.inc
@@ -250,22 +250,71 @@ function drupal_get_rdf_namespaces() {
 function drupal_get_html_head() {
...
+  $head_elements = drupal_add_html_head();
+  drupal_alter('html_head', $head_elements);
+  return drupal_render($head_elements);

Do we need to render() here or could that be deferred to the template?

+++ includes/common.inc
@@ -2502,12 +2579,32 @@ function base_path() {
+ * Use drupal_add_html_head(), drupal_add_css(), or drupal_add_js() to add
+ * multiple links with the same REL.
+++ modules/system/system.api.php
@@ -2189,6 +2189,27 @@ function hook_drupal_goto_alter(array $args) {
+ * Elements available to be altered are only those added using
+ * drupal_add_link() or drupal_add_html_head(). CSS and JS files are handled
+ * using drupal_add_css() and drupal_add_js(), so the head links for those
+ * files will not appear in the $head_elements array.

I don't really get where CSS/JS is handled, or better: extracted/removed here, so I don't really get why it isn't contained in the alter hook.

Also, if those functions shouldn't be used to touch/alter any CSS/JS, despite being also used for them, then we should move the mention of drupal_add_css/_js() and explanation about the innards into inline comments of drupal_add_html_head(), so developers don't get confused.

+++ includes/common.inc
@@ -2502,12 +2579,32 @@ function base_path() {
+function drupal_add_link($attributes, $header = FALSE) {
...
+  drupal_add_html_head($element, 'drupal_add_link-' . $attributes['rel']);

The second argument was described as "unique" for drupal_add_html_head() - so... I can only add one LINK with REL 'stylesheet' to fix my awesome IE, IE6, IE7, and IE8?

+++ includes/common.inc
@@ -2502,12 +2579,32 @@ function base_path() {
+    // Set a HTTP Link: header also.

Minor: 'Also set a HTTP header "Link:".' would be a bit more grokable.

I'm on crack. Are you, too?

@sun - render could be done in the pre/process function but since the existing API returned a string, it seemed simpler to retain that behavior.

See above re: tag vs. element

The order only matters for the one meta tag and only because of IE browser security stupidity, so there isn't a real reason to need a weight for ordering.

drupal_add_css() generates rel="stylesheet" - the point is that you should not be adding those via drupal_add_link()

Thanks for replying! However, to let this patch fly, all the clarifications need to go into the code instead. :)

The most important issue (the $key argument to drupal_add_html_head()) you didn't clarify, so I hope you will do that in the PHPDoc of the new patch.

I really think this is almost RTBC - so let's just make sure that the world outside of this issue understands how this is supposed to work and why it was done this way.

Can we additionally make drupal_get_css() use drupal_add_html_head() to make people in #576940: links tags which reference css are not themable happy? For example, by adding a $group argument (defaulting to 'meta') that's used as additional array key for the stored elements, so we'd get array('meta', 'scripts', 'styles'), allowing us to render each group separately for the template, and additionally allowing folks to alter the element HTML tag output for all these elements by overriding the theme function. May be a follow-up issue though.

A couple questions.

1) Does this patch have a change of going into D7? What's the probability?

2) Why the use of xhtml tag? I ask because everything seems to be starting the switch to html5 from xhtml. Would it be more appropriate to replace xhtml with html everywhere but use the xhtml style syntax which is compatible in html5?

We could use the theme function I define here to also format those CSS links - that would, I think, help resolve that issue.

ok, discussed more with sun. This make links unique on rel + href, and as a test, applies the new theme function to css as a start.

+++ includes/common.inc
@@ -281,22 +281,71 @@ function drupal_get_rdf_namespaces() {
+function _drupal_default_html_head() {

Why does this have an _ in front of it? Other functions like drupal_js_defaults() don't have this?

+++ includes/common.inc
@@ -2910,7 +3000,16 @@ function drupal_get_css($css = NULL) {
+        $element = array(
+          '#tag' => 'link',
+          '#attributes' => array(
+            'type' => "text/css",
+            'rel' => "stylesheet",
+            'media' => $item['media'],
+            'href' => $item['data'],
+           ),
+         );
+        $external_css .= theme('xhtml_tag', $element) . "\n";

There's an extra space on the line before the $external_css. This shows up in a couple places.

+++ includes/theme.inc
@@ -1869,6 +1869,20 @@ function theme_feed_icon($url, $title) {
 /**
+ * Generate the output for a generic XHTML tag with attributes.
+ *
+ * @ingroup themeable
+ */
+function theme_xhtml_tag(array $element) {

This should be theme_html_tag. HTML is used much more consistency in drupal than xhtml.

This may be able to be used for creating the script tag in drupal_get_js(). There is one caveat. With the script tag you can't have a closing of />. Instead you need the full closing to work.

I'm on crack. Are you, too?

oops. looks like this (or head) is broken.

@mfer - see comments above (e.g. #58) that complained that it should be xhtml not html. I honestly don't care.

_drupal_default_html_head() means it's a function for internal use - there's really no reason you'd access this as a general API function.

re: script - that will work fine if you set $element['#value'] to an empty string.

FWIW, I'm in favor of "html" over "xhtml." I think there is plenty of precedent in core for using "html" even if "xhtml" is perhaps infinitesimally more accurate.

this fixes the notices - just reorders the code in drupal_add_link()

Fixed a couple of documentation glitches.

I agree that we want to rename 'xhtml' to 'html'.

@sun - there seem to be some unrelated or incorrect changes in your patch. Please start from a clean HEAD and apply my patch.

I pulled in some of the minor wording changes.

This patch also uses the theme function to render JS tags, reverts xhtml_tag to html_tag in the theme function name.

+++ includes/common.inc
@@ -3428,18 +3534,34 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
-        $output .= '<script type="text/javascript"' . ($item['defer'] ? ' defer="defer"' : '') . '>' . $embed_prefix . $item['data'] . $embed_suffix . "</script>\n";
+        if ($item['defer']) {
+          $element['#attributes']['defer'] = 'defer';
+        }
+        $element['#value'] = $embed_prefix . $item['data'] . $embed_suffix;
+        $output .= theme('html_tag', $element);
         break;

The $embed_prefix and $embed_suffix should be part of the theme layer. They are needed to pass xhtml validation. They are not needed for html. This should be modifiable.

+++ includes/common.inc
@@ -3428,18 +3534,34 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
-          $no_preprocess .= '<script type="text/javascript"' . ($item['defer'] ? ' defer="defer"' : '') . ' src="' . file_create_url($item['data']) . ($item['cache'] ? $query_string : '?' . REQUEST_TIME) . "\"></script>\n";
+          if ($item['defer']) {
+            $element['#attributes']['defer'] = 'defer';
+          }
+          $element['#attributes']['src'] = file_create_url($item['data']) . ($item['cache'] ? $query_string : '?' . REQUEST_TIME);
+          $no_preprocess .= theme('html_tag', $element);

This change is not going to work right. Script tags can't be closed with /> in some browsers. See http://stackoverflow.com/questions/69913/why-dont-self-closing-script-ta...

This review is powered by Dreditor.

I wonder if the strangeness of how the script tag needs to operate means #576932: JavaScript not themable should be used instead.

Merged in my changes once again.

We can simply add the prefix/suffix into theme_html_tag(), which makes sense (not contained in this one).

@sun - please merge by hand or just give me the doc changes you think are needed - you keep putting un-related/old stuff in like:

+    ),
+    // Security: This always has to be output first.
+    '#weight' => -1000,

@mfer - please try the patch before complaining about the script tags - my patch outputs them in the correct form.

+++ includes/common.inc	8 Oct 2009 14:54:07 -0000
@@ -3428,18 +3540,34 @@ function drupal_get_js($scope = 'header'
     switch ($item['type']) {
       case 'setting':
-        $output .= '<script type="text/javascript">' . $embed_prefix . 'jQuery.extend(Drupal.settings, ' . drupal_json_encode(call_user_func_array('array_merge_recursive', $item['data'])) . ");" . $embed_suffix . "</script>\n";
+        $element['#value'] = $embed_prefix . 'jQuery.extend(Drupal.settings, ' . drupal_json_encode(call_user_func_array('array_merge_recursive', $item['data'])) . ");" . $embed_suffix;
+        $output .= theme('html_tag', $element);
         break;
 
       case 'inline':
-        $output .= '<script type="text/javascript"' . ($item['defer'] ? ' defer="defer"' : '') . '>' . $embed_prefix . $item['data'] . $embed_suffix . "</script>\n";
+        if ($item['defer']) {
+          $element['#attributes']['defer'] = 'defer';
+        }
+        $element['#value'] = $embed_prefix . $item['data'] . $embed_suffix;
+        $output .= theme('html_tag', $element);
         break;

When it comes to inline js the CDATA wrapper that's done with $embed_prefix and $embed_suffix should be moved to the theme layer. This is presentation stuff and some html5 developers want to be able to remove it or are writing regex in the theme layer to do it now.

This review is powered by Dreditor.

@mfer: No need to repeat the same complain all over again. Now you can go ahead and mark those other issues as duplicate.

This patch adds two custom properties #value_prefix and #value_suffix for theme_html_tag(), which works very fine.

@pwolanin: As discussed in IRC, hook_html_head_alter() is there to allow to alter the elements, so any element can contain a #weight. By assigning an insane low weight for that special meta tag, and no #weight for any other tag, this should be sufficient to make people understand. You proposed to unset all #weight properties as an alternative, but that would partially remedy the benefit of this renderable array, and seriously, we have many more alter hooks in core that equally allow to completely break your site, so I don't see the point in limiting common API functionality. The other point you mentioned about a #weight property leading to some extra cycles for sorting is performance optimization talk about microseconds.

Let's see what the testbot thinks.

Re-rolled against HEAD.

This time for real.

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

This one should pass. RTBC, anyone?

Looks pretty good to me, especially if it make a bunch of other issues duplicate.

Looks good! As an added bonus, it fixes #576940: links tags which reference css are not themable and #576932: JavaScript not themable.

The only remaining question Rob Loach raised in IRC is whether theme_html_tag() is a proper name. He asked whether he could re-use that function in some other patch. And, no, we don't want that - I made sure to add extra PHPDoc to explain that this theme function should only be used for tags in HTML HEAD.

Thus, the idea arose that it could also be named theme_html_head_tag(). But then again, that could be understood as if it would render the HEAD tag itself.

That's the only thing that may hold up this patch.

oops, alrighty :)

How about theme_html_header_tag()?

Either we rename the function, or we make "html_tag" be able to process its child elements. Then you could do something like:

$form['mycontainer'] = array(
  '#type' => 'html_tag',
  '#tag' => 'div',
  '#attributes' => array('class' => 'OMGMYCONTAINERCLASS'),
);
$form['mycontainer']['mychildelement'] = array(
  '#type' => 'textfield',
  '#title' => 'This textfield ends up being held within a <div class="OMGMYCONTAINERCLASS"></div>',
);

This could end up being follow up issue as it's cause stress to a very small, very cute, kitten. I'd vote for pimping out html_tag in a follow up issue. That's nice to the kittens, as well as considers them for nice eatings in the future.

+++ includes/common.inc	9 Oct 2009 14:00:49 -0000
@@ -281,22 +281,76 @@ function drupal_get_rdf_namespaces() {
+ * @param $key
+ *   A unique string key to allow implementations of hook_html_head_alter() to
+ *   identify the element in $data. Required if $data is not NULL.

What? Why would $data be NULL?

+++ includes/common.inc	9 Oct 2009 14:00:49 -0000
@@ -281,22 +281,76 @@ function drupal_get_rdf_namespaces() {
+  // Add default elements. Make sure the Content-Type comes first because the
+  // IE browser may be vulnerable to XSS via encoding attacks from any content
+  // that comes before this META tag, such as a TITLE tag.

It's nice to see this handled centrally in core rather than passing to the theme to get right. +100 for this.

+++ includes/common.inc	9 Oct 2009 14:00:49 -0000
@@ -2481,6 +2535,28 @@ function url($path = NULL, array $option
+ * @param $attributes
+ *   An associative array of attributes.
+ *

Like..?

+++ includes/common.inc	9 Oct 2009 14:00:49 -0000
@@ -2692,12 +2768,32 @@ function base_path() {
+ * @param $attributes
+ *   Associative array of element attributes including 'href' and 'rel'.

Can we itemize these?

+++ includes/common.inc	9 Oct 2009 14:00:49 -0000
@@ -2692,12 +2768,32 @@ function base_path() {
+ * @param $header
+ *   Optional flag to determine if a HTTP 'Link:' header should be sent.

Why might I want that? What would that do?

+++ includes/theme.inc	9 Oct 2009 01:47:51 -0000
@@ -1872,6 +1872,48 @@ function theme_feed_icon($variables) {
+ * This theme function should be used for tags appearing in the HTML HEAD of a
+ * document (specified via the #tag property in the passed $element):
...
+function theme_html_tag($variables) {

If so, then please name the function accordingly. How about theme_html_head_tag(), for semi-consistency with drupal_add_html_head()?

+++ includes/theme.inc	9 Oct 2009 01:47:51 -0000
@@ -1872,6 +1872,48 @@ function theme_feed_icon($variables) {
+ * - meta: To provide meta information.
+ * - link: To refer to stylesheets and other contextual information.
+ * - script: To load JavaScript.

First of all, it's weird to list these in the initial PHPDoc. If these are possible values for #tag, they should be listed underneath the @param description for element['#tag'].

Second of all, these descriptions need work. "meta: To provide meta information" ... what? ;)

+++ includes/theme.inc	9 Oct 2009 01:47:51 -0000
@@ -1872,6 +1872,48 @@ function theme_feed_icon($variables) {
+ *     - #value: (optional) A string containing the inner markup of the tag.

"inner markup"? Can I have an example?

+++ modules/openid/tests/openid_test.module	9 Oct 2009 01:47:51 -0000
@@ -113,7 +120,7 @@ function openid_test_html_openid1() {
-  drupal_add_html_head('<link rel="openid2.provider" href="' . url('openid-test/endpoint', array('absolute' => TRUE)) . '" />');
+  drupal_add_link(array('rel' => 'openid2.provider', 'href' => url('openid-test/endpoint', array('absolute' => TRUE))));

drupal_add_link() is not a good name for this function. I would never have expected this to add a <link> field to the <head> tag from reading this code.

Why don't we just re-use drupal_add_html_head() with an element with #tag => link, for consistency with the other non-CSS/JS properties?

If this must be its own thing, then let's at least rename the function to something like drupal_add_html_head_link().

+++ modules/system/system.api.php	9 Oct 2009 01:47:51 -0000
@@ -2256,6 +2256,27 @@ function hook_drupal_goto_alter(array $a
+ * drupal_add_link() or drupal_add_html_head(). CSS and JS files are handled
+ * using drupal_add_css() and drupal_add_js(), so the head links for those
+ * files will not appear in the $head_elements array.
...
+function hook_html_head_alter(&$head_elements) {

Hrm. So this means we have hook_js_alter(), hook_css_alter(), *and* hook_html_head_alter()? I predict this causing some DX WTFs.

I suppose we need this though, since you might want to alter JS/CSS that is not included in the <head> tags. And it would be even more of a WTF for those alter hooks to only affect CSS/JS "sometimes."

+++ modules/system/system.api.php	9 Oct 2009 01:47:51 -0000
@@ -2256,6 +2256,27 @@ function hook_drupal_goto_alter(array $a
+  foreach($head_elements as $key => $element) {

(minor) Space between foreach and (

This review is powered by Dreditor.

I'd also like to understand the performance impact of moving this stuff to renderable arrays, so we need some benchmarks.

$data has to be able to be NULL so that you can fetch the existing stored values - it's like that in D4.7, D5, D6 ...

the CSS and JS links are not part of this array - so you do not have overalapping alter hooks

As a follow-up to this issue I'd want to make the title tag rendered via this system - only then would we actually make D7 secure against the IE-7 utf-7 issue since currently the themer could still get it wrong by printing the title first.

here's a quick re-roll for a conflicts and start on fixing doxygen - doesn't address all of Angie's comments yet.

renamed the function as suggested, though it seems a little long.

renamed the function as suggested, though it seems a little long.

Re-reviewed issues mentioned by webchick - remaining todos:

+++ includes/common.inc
@@ -281,22 +281,76 @@ function drupal_get_rdf_namespaces() {
+ * @param $data
+ *   A renderable array. If the '#type' key is not set then 'html_tag' will be
+ *   added as the default '#type'.
+ * @param $key
+ *   A unique string key to allow implementations of hook_html_head_alter() to
+ *   identify the element in $data. Required if $data is not NULL.
...
+function drupal_add_html_head($data = NULL, $key = NULL) {

What? Why would $data be NULL?

+++ includes/common.inc
@@ -2503,6 +2557,28 @@ function url($path = NULL, array $options = array()) {
+ * @param $attributes
+ *   An associative array of attributes such as REL.
...
+function drupal_header_attributes(array $attributes = array()) {

Better use "'rel'" here.

+++ includes/common.inc
@@ -2736,12 +2812,32 @@ function base_path() {
+ * @param $attributes
+ *   Associative array of element attributes including 'href' and 'rel'.
...
+function drupal_add_html_head_link($attributes, $header = FALSE) {

Can we itemize these?

+++ includes/theme.inc
@@ -1872,6 +1872,48 @@ function theme_feed_icon($variables) {
 /**
+ * Generate the output for a generic HTML tag with attributes.
+ *
+ * This theme function should be used for tags appearing in the HTML HEAD of a
+ * document (specified via the #tag property in the passed $element):
...
+function theme_html_tag($variables) {

Rename to theme_html_head_tag(), for semi-consistency with drupal_add_html_head()?

+++ includes/common.inc
@@ -281,22 +281,76 @@ function drupal_get_rdf_namespaces() {
+ * @param $data
+ *   A renderable array. If the '#type' key is not set then 'html_tag' will be
+ *   added as the default '#type'.
+ * @param $key
+ *   A unique string key to allow implementations of hook_html_head_alter() to
+ *   identify the element in $data. Required if $data is not NULL.
...
+function drupal_add_html_head($data = NULL, $key = NULL) {

What? Why would $data be NULL?

I'm on crack. Are you, too?

It's not a head tag, it's a header tag. IMHO the function should be theme_html_header_tag(). Consistency is not worth confusion.

@IceCreamYou: We already use "header" with a very specific and different meaning: the HTTP request/response header. See http://api.drupal.org/api/search/7/header

We can remove the problem by making theme_html_tag have a place to render children. Then it can be used in the header or somewhere else. We could add a call to drupal_render_children and then not worry about it.

@mfer - extra calls like that when 99% not needed seem like a way to degrade performance. I agree with sun that this theme function need not be totally generic - in most cases you will want a more specific theme function.

@sun/webchick - are you saying we need doxygen about when the params can be NULL?

@sun/webchick - we cannot enumerate all possible attributes, since I'm not sure there is any cannonical list, and since this is xhtml I can make up new attributes - rel and href are the ones that are almost always going to be present.

1) @mfer: This theme function is a compromise/helper theme function for all tags in HTML HEAD. It's very unlikely that themers will want to override the output of those tags, but now they can, and they can change it for all the tags in one fell swoop. For all other theme functions, we want to use separate theme functions, so themers can override those more granularly.

2) yes, since both arguments are optional (and the second even refers to the possibility of the first being NULL), we need to document the defaults and for why I might use the defaults. As elsewhere, "By default, ... , which means...." for each @param.

3) webchick's request for listing HTML attributes for drupal_add_html_head_link() sounds strange and makes no sense to me, so let's skip that. However, we should use 'rel' instead of REL in the PHPDoc of drupal_header_attributes(), because we usually only write tag names all-uppercase, but not attributes.

fixed doxygen per sun

Excellent.

For the record, I do want to override the output of these tags. These tags are xhtml output. When I change my doctype to html I can remove a lot of what these tags spit out. I had not even through of doing this until others came to me first and asked how to do it.

For example, I don't want the cdata surrounding my inline js. That was only added to validate against xhtml. There are numerous things required in xhtml and not in html.

Expect overriding to happen. I'm a little concerned that this will end up like theme_links which is a total pain to theme because of the difference contexts it's used in. But, we can deal with that in D8.

re-roll for merge conflict in common.inc

Odd - testbot issue? Just in case, same patch rolled from a clean CVS checkout.

ah - somehow between sun and I and merging various theme patches we lost part of the patch that removed function system_preprocess_page(). So it was calling 2 non-existent theme functions and causing exceptions.

Missed the change in function name from drupal_set_header() to drupal_add_http_header() at some point recently. Correcting that in the patch for use by #attached.

My steps:

Setup:

• Checked out latest code from CVS-HEAD.
• Installed fresh Drupal site ("default" profile).
• Applied patch.
• Truncated cache tables (per pwolanin).

Test 1:

• Created an Article node, node 1, with the path "somewhere/over-the/rainbow/way-up-high".
• When it saved it took me to base_path() . "somewhere/over-the/rainbow/way-up-high" as expected. The following code was present in the HTML head:
  

  <link rel="shortlink" href="/node/1" />
  <link rel="canonical" href="/somewhere/over-the/rainbow/way-up-high" />

• Loading node/1 showed the following code in the HTML head:
  

  <link rel="shortlink" href="/node/1" />
  <link rel="canonical" href="/somewhere/over-the/rainbow/way-up-high" />

Test 2:

• Created an Article node, node 2, with no custom path.
• When it saved it took me to base_path() . "node/" . $node->nid as expected, i.e. "/node/2". The following code was present in the HTML head:
  

  <link rel="shortlink" href="/node/2" />
  <link rel="canonical" href="/node/2" />
  

Per discussion with pwolanin on IRC the noted missing tags on the homepage will be taken care of separately.

This was already rtbc, test bot is happy, DamienMcKenna 's review is good. Back to RTBC.

Further..

HTTP headers for the above tests:

http://drupal7/node/1
Link: </node/1>; rel="shortlink",</somewhere/over-the/rainbow/way-up-high>; rel="canonical"

http://drupal7/somewhere/over-the/rainbow/way-up-high
Link: </node/1>; rel="shortlink",</somewhere/over-the/rainbow/way-up-high>; rel="canonical"

http://drupal7/node/2
Link: </node/2>; rel="shortlink",</node/2>; rel="canonical"

Gah, sorry for stomping on the status ;)

note - {cache} has to be truncated to force theme registry rebuild due to stupid exception from: http://drupal.org/node/412730

just a conflict in CHANGELOG - nothing meaningful

Should we continue to discuss the individual implementation of the REL tags here or do a follow-up bug report to have it include the hostname (maybe as an option)?

New RDFa tests are failing. This probably needs a re-roll once the follow-up in #493030: RDF #1: core RDF module has been committed.

RTBC * 2. As a follow up, we could merge both Form API element types "html_tag" and the "container" that was introduced at #557272: FAPI: JavaScript States system.

@DamienMcKenna - I'd like to see the main API change in before worrying about further tweaks.

The RDF follow-up went in. Just to make sure this one still passes.

Re-rolled.

Both the re-roll in 138 and the fix in this patch were simply due to the change to hook_theme() from #600974: theme() fails for theme functions taking one argument in renderable arrays.

Re-rolled.

All right, thanks for all of the reviews and work on this patch, folks!

Committed to HEAD. We need to document the theme function additions/removals in the theme guide, plus the changes for getting an element into the head section.

yay! I'd almost given up on this :)

I'll open a follow-up to look at the title tag handling wrt IE utf-7 vulnerabilities.

@pwolanin: Please post a link to that follow-up issue here.

Small cleanup of theme_html_tag() to provide greater control over whether the tag self-closes or not, and defaulting the decision to follow http://www.w3.org/TR/xhtml1/#guidelines (C2, C3).

The patch makes sense.

However, we don't want to use this theme function for other things outside HTML HEAD, because it is very limiting for themers. It is ok to use it for the special tags, because almost no one will want to override them. Other functions that format content elsewhere should use a dedicated theme function, or at the very least use theme function suggestions with a fall-back to this one. For more information see #588148: theme_links() is not really themeable

I agree with sun - this was not really intended to be a general-purpose function. Is the output incorrect now?

Re #149: none of the code in HEAD is outputting incorrect markup, but it just seems too delicate. If someone wanted to call theme('html_tag') for a 'script' tag with a 'src' attribute, they would also have to set #value='' in order to generate valid HTML: WTF? This fixes that. Also, I just don't believe that it will be used for HEAD tags only. Why shouldn't a specific theme implementation call this theme function instead of creating its own string? Eh, maybe it'll be rare for the benefit of doing so to outweigh the performance cost of an extra theme() invocation, but that's for the contrib module/theme to decide, not for us to decide. But I updated the PHPDoc as per #148.

The latest patch makes sense, but didn't review it in detail yet. Only wanted to share an idea that just crossed my mind:

theme('html_tag__script', ...)

:)

@sun I like that idea. :)

Re-rolled. Let's get this in.

Re #152/#153: Easily done with a preprocess function:

function MYTHEME_preprocess_html_tag(&$variables) {
  $variables['theme_hook_suggestions'][] = 'html_tag__' . $variables['element']['#tag'];
}

Do we want to add a template_preprocess_html_tag() function that does this? If so, please open a separate issue for that. #154 is RTBC without that, so let's let it be.

I don't understand why this follow-up is required? Above we explicitly said this was not for theming all HTML tags, for performance among other things. Can't changing the API here wait until D8?

Maintaining that list of empty tags strikes me as a complete nightmare considering that D7 will be around for probably 3-4 years and we'll have HTML 7 by then...

I don't think #154 is an API change: it's a WTF removal. It adds an optional #empty property, but that's an addition for flexibility, not a change. XML parsers don't care whether you self-close or have a start tag and end tag, but http://www.w3.org/TR/xhtml1/#guidelines is quite clear about its recommendation for what to do to be compatible with existing user agents. So, for example, SCRIPT tags must include start and end tags even if they have no content (e.g., they use a 'src' attribute) while META tags must self-close and not use an end tag. HEAD defaults all tags to self-close, and only uses end tags when #value is set to something (possible an empty string). So, for example, everywhere where code calls theme('html_tag') for #tag='script', it needs to know about these arcane XHTML guidelines and set #value to empty string, and that's a WTF. With the patch, we implement the guidelines as defaults so calling code doesn't need to worry about this.

Because HTML 5 and other doctypes may require something other than what is recommended for XHTML 1.0, we need to provide a way to override the default. HEAD supports the override by switching on whether #value is NULL or empty string: WTF! The patch provides a #empty boolean which seems more sane. But #empty only needs to be messed with when a) it matters whether a tag self-closes or has an end tag and b) something other than XHTML 1.0 compatibility is needed.

Maintaining that list of empty tags strikes me as a complete nightmare considering that D7 will be around for probably 3-4 years and we'll have HTML 7 by then...

You over-estimate the speediness with which the W3C operates. But, true, different doctypes have different needs. For example, HTML 5 supports two syntaxes: html and xhtml. The XHTML syntax is based on XML parsing rules, so there's no distinction between a self-closing tag and start/end tags. The HTML syntax defines the HTML5-specific set of void elements. These include the same ones needed by XHTML 1.0 (minus the ones deprecated by HTML 5) plus the new ones that are not part of XHTML 1.0: command, embed, keygen, source. Since HTML 5 is still a draft, I don't think we need to add these four to the patch at this time. An html5.module can be written with:

function html5_preprocess_html_tag(&$variables) {
 if (!isset($variables['element']['#empty']) && in_array(strtolower($variables['element']['#tag']), array('command', 'embed', 'keygen', 'source')) {
   $variables['element']['#empty'] = TRUE;
 }
}

And when we decide to add built-in HTML 5 support to Drupal, we can add these 4 tags to the list in theme_html_tag(): not exactly a nightmare.

Leaving as "needs review" to see if anyone else agrees with this logic before sending back to webchick.

I agree with that explanation and I think this follow-up patch is a valuable sanitation and clarification. However, I marked this RTBC before already.

effulgentsia's explanation is accurate and the patch is straightforward and works. This is not an API change, but it does fix a bug.

I agree that it is a nice clean-up and extension, but it should wait to Drupal 8.

Patch in #154 still applies to D8. And now that D8 is open, lets see what the testbot has to say about it.

#154: drupal.theme-html-tag-empty.154.patch queued for re-testing.

... and it passes.

Re-titling to clarify what this patch does now, took a while to figure out.

Tagging issues not yet using summary template.

+, and more notes: http://www.w3.org/TR/html-polyglot/#empty-elements . I think that elements from the polyglot recommendation should be added to the list (command, embed, keygen, source) with a reference to the polyglot#empty-elements URL.

@ohnobinki: That issue was addressed in comment #157 over a year ago: http://drupal.org/node/552478#comment-2913134

I agree with @effulgentsia that those elements ought to be handled as part of the Drupal HTML5 initiative http://drupal.org/community-initiatives/drupal-core/html5#roadmap, and I don't think that shouldn't hold up this current issue.

Can we get a current issue summary please?

initial template

review done.

fix html errors

This should go in, with the HMTL5 tags mentioned by @effulgentsia in #157. It needs to be rerolled.

This changes the logic up a bit, but the functionality should be the same and is according to my limited testing. Also adds the void elements at http://www.w3.org/TR/html5/syntax.html#void-elements to the list of internally-recognized empty elements.

removed unneccessary text

Related #1825090: Remove theme_html_tag() from the theme system

changing status cause of what @thedavidmeister said :)

#type 'html_tag' still exists, the self-closing thing might still be relevant to that? I'm not sure.

added a related issue

This should have never been closed. The issue above was about converting it from a theme hook (poor choice of title for that issue btw) to an element pre_render, drupal_pre_render_html_tag() still exists.

I created #2275999: html_tag self-closes tag with NULL #value just a bit ago, but just found this one. So duplicating it in favor of this issue. I am however making this a major bug as described in that issue.

Also, we should drop the "self-closing" />. It's not necessary in HTML5 (backport patch will need to keep it though).
http://tiffanybbrown.com/2011/03/23/html5-does-not-allow-self-closing-tags/

I've been working on this and will upload a patch with tests shortly.

Tagging for http://drupaltwig.org/issues/focus

Whether or not it's required in HTML5, I still self-close empty elements with "/>". It's definitely allowed, and it makes it a lot easier to see that the element is one that can be self-closed, and doesn't just accidentally not have a closing tag.

HTML5 also doesn't require you to put quotations around attribute values, but in my opinion that way lies madness, because it leads to a lot of inconsistency between attribute values that do require quotation marks and those that don't.

If somebody wants to not self-close empty elements, that's fine. But I'd really prefer to be able to self-close an empty element if I choose. If I'm not able to do that, that would be very annoying.

Here's an article that does a good job summarizing the reasons for still using />: http://blog.teamtreehouse.com/to-close-or-not-to-close-tags-in-html5

And here's the W3C author guidelines for HTML5 that state that /> is optional (but allowed) for void elements: http://dev.w3.org/html5/html-author/#void

Again, I have no problem if some have a preference to not add in the /> on void elements. But I'd also like to have the option to do so if I so choose.

My understanding from talking with markcarver on IRC is that the primary effect here would be to change link/> to link>, and meta/> to meta>. Not how I'd choose to write it, but not the end of the world in my opinion. Both fit the spec.

In retrospect, an issue title with the word "and" denotes that it has been scope creeped, my bad. I'll open up a second issue about removing the, unnecessary, self closing /> ending. I'll update the patch here accordingly.

I've removed the removal of /> in this patch. It should be addressed in #1388926: Remove all references to "self-closing" void elements in core.

So this issue fixes the actual bug.

I'm just uploading the patch for now. I can't seem to get my interdiff to generate properly, can someone else do it?

Here is the interdiff.txt between #179 and #185.

185: html_tag_no_close_tag-552478-185.patch queued for re-testing.

Reroll of #186.

I also added a unit test class for htmlTag. This is attached separately to show the failure.

I just ran into this one in D7:S Bumping to get some reviews and for me to remember and unassigning @markcarver.

Had a review, still applies, has a great set of tests! #188 demos the problem well.

Let's get this in and finish up #1388926: Remove all references to "self-closing" void elements in core

Yep nice tests. Committed 7a2948a and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Whoever picks this up for D7 might consider creating a new issue this issue now has two commits on it - one to d7 and one to d8. Followups on the same issue suck.

Closing for a separate 7.x issue.