Having template files check variables to see if we are to inject HTML is so Drupal 5. We added hook_html_head_alter() in Drupal 7 to allow changing the mega-tags that are injected into the <head> tag. Let's remove $default_mobile_metatags in place of that in order to help clean up the template.

CommentFileSizeAuthor
#1 1616896.patch3.45 KBrobloach
html_head.patch3.71 KBrobloach

Comments

robloach’s picture

StatusFileSize
new3.45 KB

Here's the correct patch.

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community

The change looks pretty clean. Would we need a change notification for themers?

dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

robloach’s picture

I can't find a change notice for default_mobile_metatags in the first place. So I suppose we don't really need one? What are your thoughts?

johnalbin’s picture

Status: Fixed » Active

We had a rather lengthy discussion at #1468582: Add mobile friendly meta tags to the html.tpl.php about the previous approach. It was a compromise based on the disparate needs of several different types of Drupal users. The issue summary describes all the scenarios succinctly.

There was no discussion in this issue, however. I could be convinced that this patch is a good idea. But saying the previous way is "so Drupal 5" isn't much of an argument.

johnalbin’s picture

Also, we picked the best meta tag values based on our understanding and discussions, but there's still some debate about what values are the ones to use. Hiding them inside a render array is problematic, IMO.

robloach’s picture

Checking for variables in template files, and hard-coding their values in a tpl.php is a terrible idea. With this, a module (or a theme) could implement hook_html_head_alter(), and change their values. If these were hard-coded in the template file, we'd end up having to implement html.tpl.php in every theme where we'd want them changed.

They're not hidden, try putting this in bartik/template.php:

  function bartik_html_head_alter(&$head_elements) {
    debug($head_elements);
  }

The best way to learn is to look at our history. We had this problem with $scripts in the Drupal 5 days. The jQuery Update module for Drupal 5 had to do some VERY hackish stuff to solve it. This is why we added hook_js_alter(). Hard-coding things in template files is absolutely terrible, and something we must avoid.

johnalbin’s picture

Checking for variables in template files, and hard-coding their values in a tpl.php is a terrible idea.

Ok. But we didn't do that. We set a boolean variable in template_preprocess_html() and check its value in html.tpl.php. Also… ”Checking for variables in template files”… that's one of only thing a tpl file does. Templates print out variables. And sometimes they conditionally print out markup based on simple boolean variables set in preprocess functions. That pattern is everywhere in Drupal's template files. $display_submitted, $unpublished, etc…

The best way to learn is to look at our history. We had this problem with $scripts in the Drupal 5 days.

You can read the issue description of #1468582: Add mobile friendly meta tags to the html.tpl.php and see that the code you reverted did not suffer from that problem at all. We're not hardcoding a bunch of meta tags and sticking them in a variable. The meta tags are plain markup in the template file. And it was wicked simple for theme developers to modify them.

johnalbin’s picture

Title: Remove $default_mobile_metatags » Revert: "Remove $default_mobile_metatags"

Rob, I just realized the argument you made is actually one against your solution and not against the previous solution.

The html.tpl.php cannot contain render elements (this is because render elements can have js and css attachments and we need to know all of those before we can generate the html.tpl.php’s $scripts and $styles variables.) All of its renderable arrays are flattened during process hooks into plain text. So, within the html.tpl.php file, the meta tags are effectively "hard-coded" into a the $head variable.

Maybe twig will make this discussion moot, but until then, the previous method was better then having a $head variable that is impossible to modify from the html.tpl.php file.

Mobile meta tags are one of the first things people invariably want to change when creating a site. Core has no UI to edit those meta tags. Making front-end developers learn how to modify render elements right

If front-end folks could do hide($head['MobileOptimized']); and then write their own MobileOptimized meta tag in the html.tpl.php, frankly, we would have wrote the code that way to begin with. But they can't. That's why we came up with the compromise.

robloach’s picture

Another place to see this pattern solved in Drupal 8 is with the RDFa tags in html.tpl.php at #1077566: Convert html.tpl.php to HTML5. They use to be hard-coded in the template:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
  "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="<?php print $language->language; ?>" version="XHTML+RDFa 1.0" dir="<?php print $language->dir; ?>"<?php print $rdf_namespaces; ?>>

When moving to the HTML5 Doctype, instead of having an if statement to check if we want the RDFa namespaces, we used a variable for the attributes:

<!DOCTYPE html>
<html<?php print $html_attributes; ?>>

Much, much cleaner, and future proof because if someone wants to change the attributes, they don't have to hack the template. They'd just simply implement a function like rdf_preprocess_html().

If we have the mobile tags hard-coded in html.tpl.php, when you want to change the values, you'd have to re-implement html.tpl.php with the new values. Injecting variables into the templates is a better solution than hard-coding. It's cleaner, future-proof, and keeps people from hacking core.

Mobile meta tags are one of the first things people invariably want to change when creating a site. Core has no UI to edit those meta tags. Making front-end developers learn how to modify render elements right

If front-end folks could do hide($head['MobileOptimized']); and then write their own MobileOptimized meta tag in the html.tpl.php, frankly, we would have wrote the code that way to begin with. But they can't. That's why we came up with the compromise.

Maybe we need to better document hook_html_head_alter()? With this in your template.php, it is extremely easy for theme developers to change the attributes. With the attributes being hard-coded in the template, you'd need to completely re-write your html.tpl.php every-time you'd want new values.

If we had better documentation telling themers how to implement something like a bartik_html_head_alter() function to change the Mobile tags, would you be happy?

dave reid’s picture

Hard-coding these values in the .html.tpl.php makes it impossible for other modules like Metatag to override them. Thank you for switching to using the render API for now.

dave reid’s picture

Issue summary: View changes

Updated issue summary.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed 8456c62 on 8.3.x
    - Patch #1616896 by Rob Loach: remove ().
    
    

  • Dries committed 8456c62 on 8.3.x
    - Patch #1616896 by Rob Loach: remove ().
    
    

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed 8456c62 on 8.4.x
    - Patch #1616896 by Rob Loach: remove ().
    
    

  • Dries committed 8456c62 on 8.4.x
    - Patch #1616896 by Rob Loach: remove ().
    
    

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • Dries committed 8456c62 on 9.1.x
    - Patch #1616896 by Rob Loach: remove ().
    
    

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
quietone’s picture

Version: 9.3.x-dev » 8.0.x-dev
Issue summary: View changes
Status: Active » Fixed

I've read the issue and as far as I can tell this is resolved. Restoring the fix status.

If that is incorrect, reopen the issue and explain what needs to be done.

Thanks.

Status: Fixed » Closed (fixed)

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