Do not alter $variables['head'] directly. Use the API function for this, please. See http://api.drupal.org/api/function/drupal_set_html_head/6

CommentFileSizeAuthor
#25 nodewords-n588716-25.patch1.03 KBdamienmckenna
#20 preprocess_page_vars_head.patch456 bytesaether
#7 nodewords-add-tags-to-head-via-API-D6.patch941 byteshass

Comments

hass’s picture

hass commented

preprocess_page may need to be removed and code moved to hook_init(). Untested.

function nodewords_preprocess_page(&$variables) {
  list($type, $ids) = _nodewords_detect_type_and_ids();
  $variables['head'] .= nodewords_output_tags(nodewords_get_tags($type, $ids));
}
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Postponed (maintainer needs more info)

The problem with hook_init() is that is not called for cached pages; using hook_preprocess_page() makes the code simple, and avoid any security vulnerability.

Is there a reason hook_preprocess_page() should not change the content of $variables (I thought it was the purpose of that hook)?

hass’s picture

hass commented

Altering $variables in preprocess is normally ok, but it's done in a buggy way here and it's not required at all and also does not allow other modules to alter the values. Use drupal_set_html_head() only to add meta tags to the head section and to allow other modules to alter the head. It makes also sure that the last Meta tag also get's an "\n" appended (bug). Use APIs whenever possible... if not possible- you may alter $variables... but only if there is no other way.

I think it was more made to add additional variables to a page that are normally not available... for adding more custom placeholders and such things.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Use drupal_set_html_head for $variables['head'] » Use drupal_set_html_head() for $variables['head']
Status: Postponed (maintainer needs more info) » Active

The development snapshot doesn't suffer of the security vulnerability because it is able to understand when Drupal is outputting an error 403 page; if it would not implement hook_preprocess_page() it would also need to call node_access(), which is computationally too expensive (as it has been reported to me).
The suggested alternative is then not useful, as it doesn't work for cached pages.

About the new line character, specifications for HTML don't report that the character is required; you could even write the HTML code for a page using a single line, and browsers are supposed to correctly interpret it.
If then we look at the HTML generated from Drupal template, the code is not perfectly indented too, and that is not because Nodewords.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Fixed

I changed the code that now is $variables['head'] = drupal_set_html_head(nodewords_output_tags(nodewords_get_tags($type, $ids))). In this way, if any other modules would call drupal_set_html_head(), the tags added from Nodewords would not be lost.

Thanks for your report, and I apologize if I misunderstood what you were saying.

hass’s picture

hass commented
Status: Fixed » Needs work

Have you TESTED your patch BEFORE commit?

hass’s picture

hass commented
Status: Needs work » Needs review
StatusFileSize
new nodewords-add-tags-to-head-via-API-D6.patch941 bytes

Patch attached, untested, but code wise correct.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Fixed 
function drupal_set_html_head($data = NULL) {
  static $stored_head = '';

  if (!is_null($data)) {
    $stored_head .= $data ."\n";
  }
  return $stored_head;
}

Have you noticed what the function returns?

FYI, nodewords_output_tags() is supposed to return a string, which could be passed then to drupal_set_html_head(), or uses for another purpose.

The code you wrote is exactly equivalent to the code I wrote, with the difference that your code misuses a module function.

hass’s picture

hass commented
Status: Fixed » Needs review

You are abusing $variables['head'] or have not understood how drupal_set_html_head() works or need to be used.

drupal_GET_html_head() is called in the theme system and adds all head tags you have added via drupal_set_html_head() to the page template. There is one example for drupal_set_html_head() in core and many in other contrib modules - please take a look yourself.

If something may be correct that it can only be $variables['head'] = drupal_get_html_head();

 function nodewords_preprocess_page(&$variables) {
   list($type, $ids) = _nodewords_detect_type_and_ids();
   nodewords_output_tags(nodewords_get_tags($type, $ids));
   $variables['head'] = drupal_get_html_head();
 }

But the other part need to be as is and I'm not sure about the above, but I've seen it in many other modules inside hook_preprocess_page(). Your variant is not the Drupal API way.

hass’s picture

hass commented

I do not really understand why other modules doing this $variables['head'] = drupal_get_html_head() in _preprocess_page for the reason that the theme system care about this and is executed after the modules _preprocess_page... this looks all pretty useless.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

The tried the code I developed in a fresh Drupal installation, and this is the HTML output I get for the front page:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en" dir="ltr">
  <head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <link rel="shortcut icon" href="/dr61/misc/favicon.ico" type="image/x-icon" />
<link rel="canonical" href="http://localhost/dr61/" />
<meta name="revisit-after" content="1 day" />
    <title>Drupal</title>
    <link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/book/book.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/node/node.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/poll/poll.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/defaults.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/system.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/system-menus.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/user/user.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/themes/garland/style.css?z" />
<link type="text/css" rel="stylesheet" media="print" href="/dr61/themes/garland/print.css?z" />
        <!--[if lt IE 7]>
      <link type="text/css" rel="stylesheet" media="all" href="/dr61/themes/garland/fix-ie.css" />    <![endif]-->
  </head>
  <!-- Omissis -->

For what I can see, the generated output is correct, and the code doesn't alter it.
If there is anything wrong in the code, it would be nice to point that out; differently, I am not going to talk of the sex of angels.

Then, I am not going to alter the value returned from nodewords_output_tags(); therefore, if the code should really use $variables['head'] = drupal_get_html_head() (I know that function exists), then the code should be 

function nodewords_preprocess_page(&$variables) {
  list($type, $ids) = _nodewords_detect_type_and_ids();
  drupal_set_html_head(nodewords_output_tags(nodewords_get_tags($type, $ids)));
  $variables['head'] = drupal_get_html_head();
}

But to change that, I would like any evidence that the code doesn't work. I have already given prove the code is correctly working.

hass’s picture

hass commented

Do I really need to find the way when it breaks only for the reason that you do not like to use the API in the right way?

Above code IS wrong. drupal_set_html_head() expects a string and not an array! Use the patch above. It works as Drupal was designed. If this does not work we need to find out why, but your code is wrong in several ways.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Cough, cough...

This is the value returned from nodewords_output_tags():

  return implode("\n", $output);
}

Shall I take that in your PHP installation implode() returns an array?

Seriously, if there are any known issues, then I will correct the code; differently, I proved there aren't any issues with the code.

hass’s picture

hass commented
Status: Needs work » Needs review

#11 is RTBC.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

No need to add extra code that complicate things (like $variables['head']) without any need and a potential risk to make problems.

I agree; but I also think that we should be sure that the code as reported works too in every situations. The reported code was one of the options I had when changing the existing code, but I was not sure it could work in any cases.

I will test it to see if it could create problems.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work

The proposed code doesn't work. This is the output I get:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en" dir="ltr">

<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<link rel="shortcut icon" href="/dr61/misc/favicon.ico" type="image/x-icon" />
  <title>Drupal</title>
  <link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/book/book.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/node/node.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/poll/poll.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/defaults.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/system.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/system-menus.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/user/user.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/themes/bluemarine/style.css?z" />
    <script type="text/javascript"> </script>
</head>

Compare it with the following output obtained with the actual code:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en" dir="ltr">

<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
  <link rel="shortcut icon" href="/dr61/misc/favicon.ico" type="image/x-icon" />
<link rel="canonical" href="http://sunradio.local/dr61/" />
<meta name="revisit-after" content="1 day" />
  <title>Drupal</title>
  <link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/book/book.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/node/node.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/poll/poll.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/defaults.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/system.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/system/system-menus.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/modules/user/user.css?z" />
<link type="text/css" rel="stylesheet" media="all" href="/dr61/themes/bluemarine/style.css?z" />
    <script type="text/javascript"> </script>
</head>

As far as I know, an implementation of hook_preprocess_page() should not call drupal_set_html_head() without to use the return value of that function, or to use drupal_get_html_head() to populate the array index $variables['head'].

To notice that <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> appears twice in the first output, but that is not wanted, and it is caused by some Drupal code that is adding it once more (drupal_get_html_head()[1] already returns the header Content-Type together the output obtained from drupal_set_html_head()).

I think that the only solution is to change the code to:

function nodewords_preprocess_page(&$variables) {
  list($type, $ids) = _nodewords_detect_type_and_ids();
  drupal_set_html_head(nodewords_output_tags(nodewords_get_tags($type, $ids)));
  $variables['head'] = drupal_get_html_head();
}

In this way, the header Content-Type is surely added (drupal_get_html_head() is the function supposed to add that header, and if other code is adding it again, it should be changed).

[1]

function drupal_get_html_head() {
  $output = "<meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\" />\n";
  return $output . drupal_set_html_head();
}
hass’s picture

hass commented
Status: Needs review » Reviewed & tested by the community

Cross posted, #11 is correct.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

The code has been committed. Thanks for your report.

Status: Fixed » Closed (fixed)

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

aether’s picture

aether commented
Priority: Normal » Critical
Status: Closed (fixed) » Needs review
StatusFileSize
new preprocess_page_vars_head.patch456 bytes

Reopening as this issue surfaced for me when I needed to modify HTML head values in a custom module using hook_preprocess_page(). Once my preprocess hook was implemented, metatags disappeared from the HTML head. Changing nodewords_preprocess_page() to use drupal_set_html_head() as suggested in the original post corrects the issue in my case.

Instead of:

$variables['head'] = $output . "\n" . $variables['head'];

Use: 

drupal_set_html_head($output);
$variables['head'] = drupal_get_html_head();

I imagine that any other contrib modules that modify the head variable in hook_preprocess_page() may also trigger this bug and therefore this may be partially responsible for some of the "Meta tags do not appear on the page" type issues I have seen posted.

I should also note that the custom module I mention alters the head variable using the method recommended here.

Patch attached against 6.x-1.x-dev.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue tags: +v6.x-1.12 blocker

Tagging this so we can ensure it gets fixed before the next stable release.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Fixed

Makes a lot of sense and I tested to be double-sure it works. Committed to CVS.
http://drupal.org/cvs?commit=443958

Status: Fixed » Closed (fixed)

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

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Closed (fixed) » Needs work

This may need some work, given we've reverted everything back to the last changes on December 31st, 2009.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new nodewords-n588716-25.patch1.03 KB

Rerolled the patch from #20 against the latest 6.x-1.x codebase.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Fixed

This has been fixed as part of #1146174: Start re-applying patches from the queue to the new 1.x branch.

Status: Fixed » Closed (fixed)
Issue tags: -v6.x-1.12 blocker

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

Issue tags: +v6.x-1.12 blocker

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.