Don't load/cache data on 403 & 404 error pages

Thanks for catching this bug.

It might be worthwhile to extend this to cover all 403 pages and simply not load meta tags. Also, how about removing the bin option from metatag_cache_set() entirely and just hardcode it?

Thanks Kristen!

Thanks for catching this Kristen. Patch in #4 worked for me, though due to apparent complexity of the patch I'm reluctant to mark it RTBC. Also cross-posted this at #2120279: Access Denied still appears in page title after successful login due to metatag cache not being cleared as at first blush it looks like a logintoboggan issue (although it isn't).

Updated issue summary.

this does not work for me.
this is what worked. instead of checking the 403 path like in #4:

  $status = drupal_get_http_header('status');
  $is_403 = $status == '403 Forbidden';
  if (!$is_403) {
    cache_set($cid, $data, 'cache_metatag');
  }

I ll roll a patch later

and this is not just a bug when using logintoboggan:

1. Give authenticated users the permission to create article.
2. Trigger the node/add/article link as anonymous
3. Login and visit node/add/article
4. Page title is "Access denied" instead of "Create article"

Yeah we definitely should be checking the actual header and not the path.

Will this work? I've added support for 404 pages too, static cached the 403 and 404 paths (might be problematic with translated sites?), and replaced $_GET['q'] with checks against request_path() and current_path().

Duh, forgot about the global $language variable.

Also, this is related to #2062379: cache_metatag table grows too much..

Crazy question - what about just parsing the 'status' value from the array returned by drupal_get_http_header() instead, and changing it so that the value *is* cached but it'll only cache one record (when #2062379: cache_metatag table grows too much. is applied too), not multiples?

I was going to suggest caching the 403/404 results as the meta tags should be generic, but then forgot that URL fields are page-specific so they can't be cached.

How about this? It adds a check against drupal_get_http_header() before checking the site_403 and site_404 variables.

the check to drupal_get_http_header() is enough i think?
what is the usecase where the drupal_get_http_header is not set but the page is 403/404?

+++ b/metatag.module
@@ -1874,3 +1878,75 @@ function metatag_translate($name, $string, $langcode = NULL, $update = FALSE) {
+  $status = intval($headers['status']);
+  if ($status == 403 || $status == 404) {
+    return;

I would suggest not to rely on intval, but the exact strings:
404 Not Found
403 Forbidden

But even if you do, then you should do strict check here === and use typecasting (int) which is faster

Is there a real need to avoid caching 404 pages? 404 requests are often triggered by bots etc. and caching on these requests can actually be helpful.

@mfb: Yes, until we have better 404 handling.

I've added another issue related to this one: #2156261: Add a way for 403/404 pages to be customized individually

How about I add a variable to let you control whether 403/404 pages are cached, and set it to "OFF" by default? That way most sites can leave it disabled, but sites that need this can enable it.

This needs to be rerolled as I've moved most of the code into #2062379: cache_metatag table grows too much..

How's about this? Thanks to the work in #2062379: cache_metatag table grows too much. not much is actually needed. I've also added a variable 'metatag_cache_error_pages' that can be used to control whether the meta tags are cached on 403/404 pages, which is defaulted to not cache them (see the README.txt changes for details).

So the return array from drupal_get_http_header() doesn't always have a 'status' value.

Committed!

FYI the handling of 403/404 pages went through some changes in #2156261: Add a way for 403/404 pages to be customized individually.