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.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 1616896.patch | 3.45 KB | robloach |
| html_head.patch | 3.71 KB | robloach |
Comments
Comment #1
robloachHere's the correct patch.
Comment #2
Niklas Fiekas commentedThe change looks pretty clean. Would we need a change notification for themers?
Comment #3
dries commentedCommitted to 8.x. Thanks.
Comment #4
robloachI 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?
Comment #5
johnalbinWe 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.
Comment #6
johnalbinAlso, 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.
Comment #7
robloachChecking 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:The best way to learn is to look at our history. We had this problem with
$scriptsin 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.Comment #8
johnalbinOk. 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…
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.
Comment #9
johnalbinRob, 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.Comment #10
robloachAnother place to see this pattern solved in Drupal 8 is with the RDFa tags in
html.tpl.phpat #1077566: Convert html.tpl.php to HTML5. They use to be hard-coded in the template: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:
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.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?Comment #11
dave reidHard-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.
Comment #11.0
dave reidUpdated issue summary.
Comment #27
quietone commentedI'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.