| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | markup |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | API clean-up, html5, Issue summary initiative, Needs Documentation, shortlink |
Issue Summary
Problem/Motivation
Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards
- Request to add "rel=shortlink" standard so as clients can auto-detect the short links rather than having to manufacture their own.
- Request to add rel=canonical
http://drupal.org/files/issues/shortlink-uml-sequence.png
Additional Information
http://code.google.com/p/shortlink/wiki/Specification
http://googlewebmastercentral.blogspot.com/2009/02/specify-your-canonica...
Sample Header
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=shortlinkProblems
rel=shortlink is a very young specification
Proposed resolution
Seems to have been commited: #552478-154: Make self-closing optional in theme_html_tag()
Remaining tasks
#552478-157: Make self-closing optional in theme_html_tag()
Related Issues:
#552488: Add support for rel=shortlink standard D6
#567482: Allow <link> tags to be added as attached structures
Make self-closing optional in theme_html_tag()
User interface changes
API changes
It adds an optional #empty property, but that's an addition for flexibility, not a change. (not really an API change)
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| shortlink-uml-sequence.png | 70.96 KB | Ignored | None | None |
Comments
#1
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.
#2
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...
#3
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 .
#4
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.
#5
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
#6
this issue also describes part of what we are doing #567482: Allow <link> tags to be added as attached structures
#7
#8
patch written 50% by samj and by me at code sprint.
#9
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.
#10
The last submitted patch failed testing.
#11
needed an extra space in the theme function to match the expected test strings, also added some suggestions for the changelog
#12
The last submitted patch failed testing.
#13
left a bit of stray code in system.module
Also fix doxygen for function drupal_add_html_head()
#14
as a side note, this feature is already deployed on wordpress.com (including cnn.com blogs apparently) - example headers:
HTTP/1.1 200 OKServer: 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
#15
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
#16
[dupe]
#17
Now the #attached patch is in, should this be using that? Looks like a good change.
#18
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);
+ }
#19
+++ 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.
#20
If we want to allow
<link>and<meta>tags to be attached to render() structures, then we should keepdrupal_add_html_head()as an API function, which receives an array options. Then, we should work ondrupal_get_html_head()to generate a render() structure with all the data added throughdrupal_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:
<?phpfunction 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_htmlandmeta_generator_headertheme 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 todrupal_html_head_defaults().#21
The last submitted patch failed testing.
#22
- Fixed parameters in batch.inc
#23
The last submitted patch failed testing.
#24
@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().
#25
- Fixed openid tests.
#26
The last submitted patch failed testing.
#27
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.
#28
@pwolanin:
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...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.
Just to have a function where the default html head tags are added. For clarity and readability of the code.
#29
#30
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
#31
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.
#32
fixed a bug with the HTTP headers, remove a duplicate function call in system.module, improved comments, etc.
#33
This is just cleanup / condolidation on the existing patch now, but all looks fine.
#34
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
#35
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
#36
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>
#37
#38
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.
#39
missed getting $separator into drupal_attributes() actual implode()
#40
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.
#41
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.
#42
The last submitted patch failed testing.
#43
oops - left #tag_type in browser_test module
#44
#45
+++ 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.
#46
ok, minor re-roll per drupcube's suggestions.
#47
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.
#48
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()
#49
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'.
#50
No, since we still need a '; ' separator instead of ' '.
#51
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.
#52
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.
#53
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.
#54
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.
#55
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.
#56
testbot seems hung up on re-testing, so here's a re-roll of Wim's patch with no changes
#57
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.
#58
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_attributesShould 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 everywhereTotally 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!
#59
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).
#60
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?
#61
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.
#62
Re-arrange the code a little in response to Jacob, and use 'xhtml_tag' instead of 'html_tag'
#63
fix typo
#64
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
#65
The last submitted patch failed testing.
#66
very minor conflict - re-rolled.
#67
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?
#68
@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()
#69
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.
#70
The last submitted patch failed testing.
#71
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.
#72
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?
#73
We could use the theme function I define here to also format those CSS links - that would, I think, help resolve that issue.
#74
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.
#75
The last submitted patch failed testing.
#76
+++ 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?
#77
oops. looks like this (or head) is broken.
#78
@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.
#79
re: script - that will work fine if you set
$element['#value']to an empty string.#80
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.
#81
this fixes the notices - just reorders the code in drupal_add_link()
#82
Fixed a couple of documentation glitches.
I agree that we want to rename 'xhtml' to 'html'.
#83
@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.
#84
+++ 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.
#85
I wonder if the strangeness of how the script tag needs to operate means #576932: JavaScript not themable should be used instead.
#86
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).
#87
@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.
#88
+++ 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.
#89
@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.
#90
The last submitted patch failed testing.
#91
Re-rolled against HEAD.
#92
The last submitted patch failed testing.
#93
This time for real.
#94
The last submitted patch failed testing.
#95
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.
#96
This one should pass. RTBC, anyone?
#97
Looks pretty good to me, especially if it make a bunch of other issues duplicate.
#98
Looks good! As an added bonus, it fixes #576940: links tags which reference css are not themable and #576932: JavaScript not themable.
#99
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.
#100
oops, alrighty :)
#101
How about theme_html_header_tag()?
#102
Either we rename the function, or we make "html_tag" be able to process its child elements. Then you could do something like:
<?php$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.
#103
+++ 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.
#104
I'd also like to understand the performance impact of moving this stuff to renderable arrays, so we need some benchmarks.
#105
$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.
#106
here's a quick re-roll for a conflicts and start on fixing doxygen - doesn't address all of Angie's comments yet.
#107
renamed the function as suggested, though it seems a little long.
#108
renamed the function as suggested, though it seems a little long.
#109
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?
#110
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.
#111
@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
#112
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.
#113
@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.
#114
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.
#115
fixed doxygen per sun
#116
Excellent.
#117
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.
#118
re-roll for merge conflict in common.inc
#119
The last submitted patch failed testing.
#120
Odd - testbot issue? Just in case, same patch rolled from a clean CVS checkout.
#121
The last submitted patch failed testing.
#122
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.
#123
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.
#124
My steps:
Setup:
Test 1:
<link rel="shortlink" href="/node/1" /><link rel="canonical" href="/somewhere/over-the/rainbow/way-up-high" />
<link rel="shortlink" href="/node/1" /><link rel="canonical" href="/somewhere/over-the/rainbow/way-up-high" />
Test 2:
<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.
#125
This was already rtbc, test bot is happy, DamienMcKenna 's review is good. Back to RTBC.
#126
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"#127
Gah, sorry for stomping on the status ;)
#128
note - {cache} has to be truncated to force theme registry rebuild due to stupid exception from: http://drupal.org/node/412730
#129
The last submitted patch failed testing.
#130
just a conflict in CHANGELOG - nothing meaningful
#131
The last submitted patch failed testing.
#132
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)?
#133
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.
#134
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.
#135
@DamienMcKenna - I'd like to see the main API change in before worrying about further tweaks.
#136
The RDF follow-up went in. Just to make sure this one still passes.
#137
The last submitted patch failed testing.
#138
Re-rolled.
#139
The last submitted patch failed testing.
#140
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.
#141
The last submitted patch failed testing.
#142
Re-rolled.
#143
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.
#144
#145
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.
#146
@pwolanin: Please post a link to that follow-up issue here.
#147
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).
#148
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
#149
I agree with sun - this was not really intended to be a general-purpose function. Is the output incorrect now?
#150
Re-test of theme_html_tag_cleanup-552478-147.patch from comment #147 was requested by sun.
#151
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.
#152
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', ...):)
#153
@sun I like that idea. :)
#154
Re-rolled. Let's get this in.
#155
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.
#156
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...
#157
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.
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.
#158
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.
#159
effulgentsia's explanation is accurate and the patch is straightforward and works. This is not an API change, but it does fix a bug.
#160
I agree that it is a nice clean-up and extension, but it should wait to Drupal 8.
#161
Patch in #154 still applies to D8. And now that D8 is open, lets see what the testbot has to say about it.
#162
#154: drupal.theme-html-tag-empty.154.patch queued for re-testing.
#163
... and it passes.
#164
Re-titling to clarify what this patch does now, took a while to figure out.
#165
Tagging issues not yet using summary template.
#166
+, 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.
#167
@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.
#168
Can we get a current issue summary please?
#169
This should go in, with the HMTL5 tags mentioned by @effulgentsia in #157. It needs to be rerolled.
#170
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.