Updated: #174

Problem/Motivation

drupal_pre_render_html_tag() is not specific in allowing which tags can be void (no closing tag).

Proposed resolution

Only allow void elements to have no closing tag.

Remaining tasks

None

User interface changes

None

API changes

None.

Original report by @samj

Please add support for the rel=shortlink standard so as clients can auto-detect the short links rather than having to manufacture their own.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because elements that are not intended to self-close, do if their value is empty.
Issue priority Major because it causes unintentional markup bugs.
Prioritized changes The main goal of this issue is DX and TX for markup generation
Files: 
CommentFileSizeAuthor
#188 self-closing-tags-552478-188.patch7.74 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,229 pass(es). View
#188 self-closing-tags-552478-188-will-fail.patch4.04 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,198 pass(es), 1 fail(s), and 0 exception(s). View

Comments

IceCreamYou’s picture

Status: Active » Closed (won't fix)

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.

pwolanin’s picture

Project: Shorten URLs » Drupal core
Version: 6.x-1.x-dev » 7.x-dev
Component: Code » base system
Status: Closed (won't fix) » Active

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...

RobLoach’s picture

Status: Active » Closed (works as designed)

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 .

IceCreamYou’s picture

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.

pwolanin’s picture

Title: Add support for rel=shortlink standard » Improve link/header API and support on all pages rel=canonical and rel=shortlink standards
Status: Closed (works as designed) » Active

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

pwolanin’s picture

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

pwolanin’s picture

Title: Improve link/header API and support on all pages rel=canonical and rel=shortlink standards » Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards
pwolanin’s picture

Status: Active » Needs review
FileSize
9.89 KB
Failed: 12788 passes, 3 fails, 0 exceptions View

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

pwolanin’s picture

Issue tags: +API change

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.88 KB
Failed: 12769 passes, 314 fails, 18087 exceptions View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

FileSize
11.14 KB
Failed: Failed to apply patch. View

left a bit of stray code in system.module

Also fix doxygen for function drupal_add_html_head()

pwolanin’s picture

Status: Needs work » Needs review

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
samj’s picture

Thanks for updating the issue with the patch (and for being one of the last to leave DrupalCon Paris to get it done!).

FWIW according to the wp.me release commentary it will be added to the Wordpress.org stats plugin too (for those who prefer to host their own blogs).

A growing number of commercial sites have added support too (eg PHP.net docs, Ars Technica) and support has been requested from many (eg Twitter) clients.

Sam on iPhone

samj’s picture

[dupe]

catch’s picture

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

samj’s picture

I believe we are (in fact this was one of the motivators for pushing #attached yesterday). For example:

+ if ($header) {
+ $head_element['#attached']['drupal_set_link_header'][] = array($attributes);
+ }

dropcube’s picture

Status: Needs review » Needs work
+++ 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.

dropcube’s picture

Status: Needs work » Needs review
FileSize
15.17 KB
Failed: Failed to install HEAD. View

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().

Status: Needs review » Needs work

The last submitted patch failed testing.

dropcube’s picture

Status: Needs work » Needs review
FileSize
15.21 KB
Failed: 12819 passes, 9 fails, 0 exceptions View

- Fixed parameters in batch.inc

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

@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().

dropcube’s picture

Status: Needs work » Needs review
FileSize
15.21 KB
Failed: Failed to install HEAD. View

- Fixed openid tests.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review

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.

dropcube’s picture

Status: Needs review » Needs work

@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.

dropcube’s picture

Status: Needs work » Needs review
FileSize
15.25 KB
Failed: Failed to apply patch. View
pwolanin’s picture

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

pwolanin’s picture

FileSize
14.33 KB
Failed: Failed to apply patch. View

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.

pwolanin’s picture

FileSize
14.54 KB
Failed: Failed to apply patch. View

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

catch’s picture

Status: Needs review » Reviewed & tested by the community

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

EclipseGc’s picture

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

samj’s picture

Thanks for your respective efforts - I was sure on Saturday these ideas wouldn't see the light of day and have been blown away by the efficiency and professionalism of the Drupal community in getting this done.

Resolving a big headache for webmasters (rel=canonical), giving users an alternative to linkrot-inducing third-party shorteners (rel=shortlink) AND providing a structured interface to both HTTP and HTML metadata is a big win.

Sam

webchick’s picture

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>

webchick’s picture

Status: Reviewed & tested by the community » Needs work
pwolanin’s picture

Status: Needs work » Needs review
FileSize
15.57 KB
Failed: Failed to apply patch. View

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.

pwolanin’s picture

FileSize
15.78 KB
Failed: Failed to apply patch. View

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

pwolanin’s picture

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.

pwolanin’s picture

FileSize
14.77 KB
Failed: 12880 passes, 2 fails, 0 exceptions View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Postponed (maintainer needs more info)
FileSize
14.77 KB
Failed: Failed to apply patch. View

oops - left #tag_type in browser_test module

pwolanin’s picture

Status: Postponed (maintainer needs more info) » Needs review
dropcube’s picture

+++ 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.

pwolanin’s picture

FileSize
15.02 KB
Failed: Failed to apply patch. View

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

Damien Tournoud’s picture

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.

pwolanin’s picture

FileSize
15.1 KB
Failed: Failed to apply patch. View

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()

catch’s picture

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'.

pwolanin’s picture

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

moshe weitzman’s picture

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.

pwolanin’s picture

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.

moshe weitzman’s picture

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.

pwolanin’s picture

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.

Wim Leers’s picture

FileSize
15.1 KB
Failed: Failed to apply patch. View

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.

pwolanin’s picture

FileSize
15.1 KB
Failed: Failed to apply patch. View

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

pwolanin’s picture

FileSize
16.68 KB
Failed: Failed to apply patch. View

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.

JacobSingh’s picture

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!

pwolanin’s picture

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).

c960657’s picture

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?

pwolanin’s picture

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.

pwolanin’s picture

FileSize
16.74 KB
Failed: Failed to apply patch. View

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

pwolanin’s picture

FileSize
16.74 KB
Failed: Failed to apply patch. View

fix typo

samj’s picture

In consideration of feedback from webchick, c960657 and others I have just posted draft-johnston-addressing-link-relations (attached) to the IETF for formal discussion/standardisation. It formally specifies rel=shortlink and rel=canonical (among others). The preliminary discussions based on the original rel=shortlink specification were encouraging and a number of other use cases identified have also been rolled into the draft. Remember we need both rough consensus and running code to have an Internet standard so there's not much point in saying it's "very young" or it always will be - use on 100+ million pages under wordpress.com goes a long way towards proving its safety, if not its utility.

The problem of third-party URL shortening services is growing at a disturbing rate - there are now literally thousands of these services with many more popping up (and dying off) every single day and the resulting linkrot is the Internet equivalent of rust in the real world. Most disconcerting is that the vast majority of interesting discussion has moved from the blogosphere (which is relatively immune to linkrot) to microblogs (which are pretty much a black hole for search engines and statistical analysis). There is little or no downside and significant upside to giving webmasters control over their addressing, not to mention the benefit to users from improving performance and making links transparent again (and bringing an end to a wave of rick rolling and worse in the process). Of course if you think you've identified a risk then we're all ears but pretty much everyone I've discussed it with over the last 6 months has been overwhelmingly positive about it.

c960657: using the same domain is a feature, not a bug. With third-party URL shorteners typically adding hundreds of milliseconds to page render times, being able to avoid another recursive DNS resolution, another TCP 3-way handshake, possibly another SSL handshake and another HTTP request means that the shortening overhead is reduced from a significant percentage to a negligible amount. Also see DJB's costs and benefits of third-party DNS service article which is a similar issue to third-party shortening. Furthermore, the intention of the API is that modules can hook in to use shorter domains, intelligible slugs or even third-party services, while all users derive some benefit from inclusion in core.

Sam

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
16.2 KB
Failed: Failed to apply patch. View

very minor conflict - re-rolled.

sun’s picture

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?

pwolanin’s picture

@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()

sun’s picture

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

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.

mfer’s picture

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?

pwolanin’s picture

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
17.12 KB
Failed: 13676 passes, 0 fails, 566 exceptions View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

mfer’s picture

Status: Needs work » Needs review
+++ 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?

mfer’s picture

Status: Needs review » Needs work

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

pwolanin’s picture

Status: Needs work » Needs review

@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.

pwolanin’s picture

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

IceCreamYou’s picture

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.

pwolanin’s picture

FileSize
17.13 KB
Failed: Failed to apply patch. View

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

sun’s picture

Category: feature » task
Issue tags: +API clean-up
FileSize
18.98 KB
Failed: Failed to apply patch. View

Fixed a couple of documentation glitches.

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

pwolanin’s picture

FileSize
21.38 KB
Failed: Failed to apply patch. View

@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.

mfer’s picture

Status: Needs review » Needs work
+++ 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.

mfer’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
23.2 KB
Failed: Failed to apply patch. View

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).

pwolanin’s picture

@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.

mfer’s picture

+++ 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.

sun’s picture

FileSize
25.38 KB
Failed: 13534 passes, 10 fails, 0 exceptions View

@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.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
25.55 KB
Failed: Failed to install HEAD. View

Re-rolled against HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
25.73 KB
Failed: 13545 passes, 1 fail, 0 exceptions View

This time for real.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Issue tags: +D7 API clean-up

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.

sun’s picture

Status: Needs work » Needs review
FileSize
26.59 KB
Failed: Failed to apply patch. View

This one should pass. RTBC, anyone?

pwolanin’s picture

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

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Status: Reviewed & tested by the community » Needs review

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.

sun’s picture

Status: Needs review » Reviewed & tested by the community

oops, alrighty :)

IceCreamYou’s picture

How about theme_html_header_tag()?

RobLoach’s picture

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 
', );

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

webchick’s picture

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

pwolanin’s picture

$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.

pwolanin’s picture

FileSize
25 KB
Failed: Failed to run tests. View

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

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: -shortlink, -API change, -API clean-up, -D7 API clean-up

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

pwolanin’s picture

Issue tags: +shortlink, +API change, +API clean-up, +D7 API clean-up
FileSize
26.5 KB
Failed: Failed to run tests. View

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

sun’s picture

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?

IceCreamYou’s picture

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.

sun’s picture

@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

mfer’s picture

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.

pwolanin’s picture

@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.

sun’s picture

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.

pwolanin’s picture

FileSize
26.66 KB
Failed: Failed to run tests. View

fixed doxygen per sun

sun’s picture

Status: Needs review » Reviewed & tested by the community

Excellent.

mfer’s picture

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.

pwolanin’s picture

FileSize
26.7 KB
Failed: Failed to run tests. View

re-roll for merge conflict in common.inc

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
29 KB
Failed: Failed to run tests. View

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
27.29 KB
Failed: 13272 passes, 1 fail, 0 exceptions View

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.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
27.33 KB
Failed: Failed to apply patch. View

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.

DamienMcKenna’s picture

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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

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

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Needs review

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"

DamienMcKenna’s picture

Status: Needs review » Reviewed & tested by the community

Gah, sorry for stomping on the status ;)

pwolanin’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
27.37 KB
Failed: 14433 passes, 3 fails, 0 exceptions View

just a conflict in CHANGELOG - nothing meaningful

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

DamienMcKenna’s picture

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)?

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
30.6 KB
Failed: Failed to apply patch. View

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.

RobLoach’s picture

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.

pwolanin’s picture

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

sun’s picture

FileSize
30.6 KB
Failed: Failed to apply patch. View

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
30.53 KB
Failed: Failed to install HEAD. View

Re-rolled.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
30.52 KB
Failed: Failed to apply patch. View

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
30.52 KB
Passed: 14465 passes, 0 fails, 0 exceptions View

Re-rolled.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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.

webchick’s picture

pwolanin’s picture

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.

sun’s picture

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

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
Passed on all environments. View

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).

sun’s picture

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

pwolanin’s picture

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

Re-test of theme_html_tag_cleanup-552478-147.patch from comment #147 was requested by sun.

effulgentsia’s picture

FileSize
3.29 KB
PASSED: [[SimpleTest]]: [MySQL] 17,639 pass(es). View

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.

sun’s picture

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', ...)

:)

mfer’s picture

@sun I like that idea. :)

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.73 KB
PASSED: [[SimpleTest]]: [MySQL] 29,853 pass(es). View

Re-rolled. Let's get this in.

effulgentsia’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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...

effulgentsia’s picture

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.

sun’s picture

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.

IceCreamYou’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Version: 7.x-dev » 8.x-dev

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

TR’s picture

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

TR’s picture

TR’s picture

... and it passes.

catch’s picture

Title: Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards » Make self-closing optional in theme_html_tag()

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

xjm’s picture

Tagging issues not yet using summary template.

ohnobinki’s picture

Status: Reviewed & tested by the community » Needs work

+, 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.

TR’s picture

Status: Needs work » Reviewed & tested by the community

@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.

Everett Zufelt’s picture

Component: base system » markup
Status: Reviewed & tested by the community » Needs work
Issue tags: +html5

Can we get a current issue summary please?

marcoka’s picture

Issue summary: View changes

initial template

marcoka’s picture

Issue summary: View changes

review done.

marcoka’s picture

Issue summary: View changes

fix html errors

Jacine’s picture

Issue tags: -API change

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

ohnobinki’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 36,247 pass(es). View

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.

ohnobinki’s picture

Issue summary: View changes

removed unneccessary text

thedavidmeister’s picture

jenlampton’s picture

Status: Needs review » Closed (won't fix)

changing status cause of what @thedavidmeister said :)

thedavidmeister’s picture

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

thedavidmeister’s picture

Issue summary: View changes

added a related issue

markcarver’s picture

Title: Make self-closing optional in theme_html_tag() » Remove "self-closing" and restrict no "closing tags" to only void elements in html_tag
Assigned: Unassigned » markcarver
Category: Task » Bug report
Priority: Normal » Major
Status: Closed (won't fix) » Needs work
Issue tags: -shortlink +Needs tests, +needs backport to D7, +theme system cleanup

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.

markcarver’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs Documentation, -Needs tests
Related issues: +#2275999: html_tag self-closes tag with NULL #value, +#1825090: Remove theme_html_tag() from the theme system, +#1268180: Newline character in HtmlTag causes unwanted whitespace
FileSize
2.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,692 pass(es), 3 fail(s), and 0 exception(s). View
3.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,690 pass(es), 1 fail(s), and 0 exception(s). View
markcarver’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 175: html_tag_no_close_tag-552478-175.patch, failed testing.

The last submitted patch, 175: html_tag_no_close_tag-552478-175-tests.patch, failed testing.

markcarver’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,677 pass(es). View
1.01 KB
markcarver’s picture

Issue tags: +Twig, +sprint
mdrummond’s picture

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.

mdrummond’s picture

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.

mdrummond’s picture

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.

markcarver’s picture

Title: Remove "self-closing" and restrict no "closing tags" to only void elements in html_tag » Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag

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.

markcarver’s picture

FileSize
3.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,794 pass(es). View

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?

markcarver’s picture

Issue summary: View changes
FileSize
2.57 KB

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

markcarver’s picture

jhedstrom’s picture

FileSize
4.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,198 pass(es), 1 fail(s), and 0 exception(s). View
7.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,229 pass(es). View

Reroll of #186.

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

The last submitted patch, 188: self-closing-tags-552478-188-will-fail.patch, failed testing.

joelpittet’s picture

Assigned: markcarver » Unassigned

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

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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

  • alexpott committed 7a2948a on 8.0.x
    Issue #552478 by markcarver, sun, effulgentsia, jhedstrom, ohnobinki:...
alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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.

  • alexpott committed 7a2948a on 8.1.x
    Issue #552478 by markcarver, sun, effulgentsia, jhedstrom, ohnobinki:...

  • webchick committed 0d8515d on 8.3.x
    #552478 by pwolanin, samj, dropcube, and sun: Improve link/header API...
  • alexpott committed 7a2948a on 8.3.x
    Issue #552478 by markcarver, sun, effulgentsia, jhedstrom, ohnobinki:...

  • webchick committed 0d8515d on 8.3.x
    #552478 by pwolanin, samj, dropcube, and sun: Improve link/header API...
  • alexpott committed 7a2948a on 8.3.x
    Issue #552478 by markcarver, sun, effulgentsia, jhedstrom, ohnobinki:...

  • webchick committed 0d8515d on 8.4.x
    #552478 by pwolanin, samj, dropcube, and sun: Improve link/header API...
  • alexpott committed 7a2948a on 8.4.x
    Issue #552478 by markcarver, sun, effulgentsia, jhedstrom, ohnobinki:...

  • webchick committed 0d8515d on 8.4.x
    #552478 by pwolanin, samj, dropcube, and sun: Improve link/header API...
  • alexpott committed 7a2948a on 8.4.x
    Issue #552478 by markcarver, sun, effulgentsia, jhedstrom, ohnobinki:...