Recommend commit message:

Issue #1934508 by jwilson3, wesleydv, matglas86, longwave, sidharthap, superspring, clemens.tolboom, MegaChriz: Make cache clear affect logo and favicon.

Problem/Motivation

Drupal has a great piece of functionality for front-end developers to force browsers to reload css and javascript. However, this same courtesy is not extended to the favicon and the logo image.

Developers using a git/svn based deployment process cannot overwrite the logo.png or favicon.ico files in a theme, deploy into a production environment, and expect the site to display the updated files because browsers that have already visited the site will pull these files from their cache.

From a front-end developer perspective, theme_get_setting does not provide any clean method to alter the data in order to add a cache-busting query string from a custom module or theme, thus the only way to ensure all browsers get a new version of these files is to either specify a separate path to a different logo or favicon file or upload a file with a different name. This can be done only via the Appearance page in the Drupal administrative user interface or by writing an update script in a custom module.

Proposed resolution

The proposed solution here appends the standard Drupal browser cache-busting string (css_js_query_string) to the logo and favicon URLs generated in theme_get_setting.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#60 interdiff-1934508.txt1.3 KBlongwave
#60 1934508-cache-clear-logo-60.patch5.24 KBlongwave
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,646 pass(es).
[ View ]
#37 1934508-favicon-37.patch4.21 KBwesleydv
PASSED: [[SimpleTest]]: [MySQL] 58,776 pass(es).
[ View ]
#37 1934508-favicon-37-interdiff.txt644 byteswesleydv
#35 1934508-favicon-35.patch4.21 KBwesleydv
PASSED: [[SimpleTest]]: [MySQL] 58,740 pass(es).
[ View ]
#35 1934508-favicon-35-interdiff.txt586 byteswesleydv
#33 1934508-favicon-33.patch4.2 KBwesleydv
FAILED: [[SimpleTest]]: [MySQL] 58,251 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#33 1934508-favicon-30-interdiff.txt573 byteswesleydv
#30 1934508-favicon-30.patch4.19 KBwesleydv
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#23 1934508-favicon-23.patch5.12 KBmatglas86
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934508-favicon-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 1934508-favicon-23-interdiff.txt0 bytesmatglas86
#19 1934508-favicon-19.patch4.94 KBmatglas86
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#19 1934508-favicon-19-interdiff.txt639 bytesmatglas86
#9 cache-clear-doesnt-affect-logo-1934508-interdiff-2-7.txt2.46 KBclemens.tolboom
#7 drupal-cache-controlled-logo-and-favicon-1934508-7.patch4.93 KBMegaChriz
PASSED: [[SimpleTest]]: [MySQL] 53,370 pass(es).
[ View ]
#3 1934508-cache-controlled-logo-and-favicon-d7-do-not-test.patch2.66 KBjwilson3
#2 1934508-cache-controlled-logo-and-favicon-2.patch2.66 KBjwilson3
FAILED: [[SimpleTest]]: [MySQL] 52,559 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#1 1934508-cache-controlled-logo-and-favicon.patch2.66 KBjwilson3
FAILED: [[SimpleTest]]: [MySQL] 52,277 pass(es), 34 fail(s), and 9,980 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.66 KB
FAILED: [[SimpleTest]]: [MySQL] 52,277 pass(es), 34 fail(s), and 9,980 exception(s).
[ View ]

StatusFileSize
new2.66 KB
FAILED: [[SimpleTest]]: [MySQL] 52,559 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

+++ b/core/includes/theme.incundefined
@@ -1436,32 +1436,48 @@ function theme_get_setting($setting_name, $theme = NULL) {
+          $cache[$theme]['logo'] .= $logo_path . $query_string_separator . $query_string;

this should be an assignment operator not a concatenation.

fixed in this patch.

Status:Needs review» Needs work

The last submitted patch, 1934508-cache-controlled-logo-and-favicon-2.patch, failed testing.

The test seems to make no different between 'filename.png' and 'filename.png?abc' which is the whole issue here, to let the browser know that there is a difference between those files so the browsers will not use their agressive browser caching on the favicon.

patch in #2 works locally and as intended. See #5 for why test may fail.

Status:Needs work» Needs review
StatusFileSize
new4.93 KB
PASSED: [[SimpleTest]]: [MySQL] 53,370 pass(es).
[ View ]

In addition of the patch in #2, this patch just simply patches the Drupal\system\Tests\System\ThemeTest and adds the query string to the expected image urls in the test.

Thanks for following up with the tests! I saw the failures but didnt get a chance to fix them myself earlier this week. Lets see how it turns out... (still yellow).

Added interdiff-2-7

I am not sure how to test this. The logo path http://drupal.d8/sites/default/files/logo.png?mjg1v7 looks ok.

You can test the first patch by applying it, clear cache and open up the html source and look for the favicon.ico and logo.png.

@richardj sorry I was not clear enough.

I've tested the logo right? But I'm not a themer so am not sure how to test this exactly. Browser quirks etc.

The tests pass and the code looks good to me. So as for me it's RTBC :)

Status:Needs review» Reviewed & tested by the community

@clemens.tolboom There are no browser quirks involved in this issue, because the essential image doesn't change, what is done here is essentially what happens with CSS and JS files (when you check the source you see that they also use query strings to indicate a certain 'version'). So the output to the browser / user stays the same. I hope that clears things up a bit.

It looks good to me as well, so RTBC!

Status:Reviewed & tested by the community» Needs work

+++ b/core/includes/theme.incundefined
@@ -1437,32 +1437,48 @@ function theme_get_setting($setting_name, $theme = NULL) {
+      // A dummy query-string is added to filenames, to gain control over
+      // browser-caching. The string changes on every update or full cache
+      // flush, forcing browsers to load a new copy of the files, as the
+      // URL changed.
+      $query_string = variable_get('css_js_query_string', '0');
+++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.phpundefined
@@ -48,31 +48,34 @@ function testThemeSettings() {
+    // Logo's and favicons receive a query string.
+    $query_string = variable_get('css_js_query_string', '0');

We should use config() instead of variable_get(), since the latter is deprecated.

Also -- I might be mistaken, but isn't this an optional configuration? If so, we might want to re-think the way we're changing the tests here. (I could be wrong.)

Category:bug» feature
Priority:Minor» Normal

Also, while this is a great change, I think it's more of a feature request.

I think this can be considered a feature, but it is something like caching / compressing css and javascript, quite essential.

This is something small that doesn't really benefit from having a on / off switch, but it benefits from being at least there.

Agree with #16, to grant these two elements their own on/off cache control switches on the theme general settings (or in the admin performance page) seems overkill -- this is a tiny "silent" feature for themers that should just work. I suppose there could be a variable that if set in your settings.php file, would specifically exclude the query string from the favicon and icon, if you so desire, but really, whats the point?

StatusFileSize
new639 bytes
new4.94 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I replace the variable_get with the state()-get() as suggested in #17.

Status:Needs work» Needs review

Ooh I forgot to set it to needs review.

Status:Needs review» Needs work

The last submitted patch, 1934508-favicon-19.patch, failed testing.

Agree with #16, to grant these two elements their own on/off cache control switches on the theme general settings (or in the admin performance page) seems overkill -- this is a tiny "silent" feature for themers that should just work. I suppose there could be a variable that if set in your settings.php file, would specifically exclude the query string from the favicon and icon, if you so desire, but really, whats the point?

Fair enough. Thanks!

Status:Needs work» Needs review
StatusFileSize
new0 bytes
new5.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934508-favicon-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I figured that there is a problem with in the install process and getting state(). So this is no surrounded with try catch. That was the result of the test. It should be working now.

Issue tags:-Needs themer review

#23: 1934508-favicon-23.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs themer review

The last submitted patch, 1934508-favicon-23.patch, failed testing.

Edit: Removed useless rant ;O

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Needs reroll

patch in #23 needs a reroll.

StatusFileSize
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Reroll against head

Status:Needs work» Needs review
Issue tags:-Needs reroll

Status:Needs review» Needs work

The last submitted patch, 1934508-favicon-30.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new573 bytes
new4.2 KB
FAILED: [[SimpleTest]]: [MySQL] 58,251 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixing issue from patch #30

Status:Needs review» Needs work

The last submitted patch, 1934508-favicon-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new586 bytes
new4.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,740 pass(es).
[ View ]

Fixing issue in ThemeTest.php

Status:Needs review» Needs work

Sorry for this nitpick, but:

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
@@ -49,31 +49,39 @@ function testThemeSettings() {
+    // Logo's and favicons receive a query string.

Incorrect use of apostrophe. Looks like this got introduced way back in #7, sorry I didn't catch this earlier.

Otherwise, it looks great to me.

Status:Needs work» Needs review
StatusFileSize
new644 bytes
new4.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,776 pass(es).
[ View ]

Fixed typo described in #36

StatusFileSize
new3.26 KB
new5.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,982 pass(es).
[ View ]

Thanks for that fix.

  1. +++ b/core/includes/theme.inc
    @@ -1422,6 +1434,10 @@ function theme_get_setting($setting_name, $theme = NULL) {
    +        if (!empty($favicon_path)) {
    +          $query_string_separator = (strpos($favicon_path, '?') !== FALSE) ? '&' : '?';
    +          $cache[$theme]['favicon'] = $favicon_path . $query_string_separator . $query_string;
    +        }

    This is broken code $cache[$theme]['favicon'] if this code is ever reached, it won't work and will cause a fatal error "Fatal error: Cannot use object of type Drupal\Core\Theme\ThemeSettings as array".

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
    @@ -49,31 +49,39 @@ function testThemeSettings() {
    +    // Logos and favicons receive a query string.

    Code comments should use the hyphenated text "query-string".

  3. From a manual test of #37 using spark theme, I can see that neither the default favicon nor the default logo image receive the query-string, and only images that are uploaded via the UI get the query string. This defeats the entire reason for filing this issue, which is to serve the folks who want to update their theme's default logo.png and favicon.ico files (inside their theme folder) without needing to upload new files via Drupal's admin UI. The idea is you update your theme asset files in the file system via FTP, version control, or other code deployment mechanism, then clear the Drupal cache, and Done.

I've refactored the code to address all three of these issues, since there is a bit of structural flow in the if statements, its probably best to apply the patch and read through it, instead of trying to figure out whats going on just reading the patch.

+++ b/core/includes/theme.inc
@@ -1395,30 +1395,47 @@ function theme_get_setting($setting_name, $theme = NULL) {
+          if (!file_exists($logo_path = dirname($theme_object->filename) . '/logo.png')) {
+            $logo_path = FALSE;
+          }
...
+          if (!file_exists($favicon_path = dirname($theme_object->filename) . '/favicon.ico')) {
+            $favicon_path = 'core/misc/favicon.ico';
           }

If the theme is using default favicon and a favicon.ico file is not provided in the theme, then it uses Drupal's core favicon.

However, if the theme is using the default logo and logo.png doesn't exist in the theme, then it displays an img tag with an empty src attribute.

This should be consistent.

We should make the default logo_path be 'core/misc/druplicon.png' if no logo is found in the theme.

Status:Needs review» Needs work

Tagging in case anyone wants to grab this and roll in #39.

Status:Needs work» Needs review
StatusFileSize
new5.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,841 pass(es).
[ View ]

This is taking the patch from #38 and applying the review in #39

Issue summary:View changes

Clarified first paragraph

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

Tested that it has the correct behaviour and implements the last remarks

Issue summary:View changes

I've cleaned up the issue summary.

Issue summary:View changes

Status:Reviewed & tested by the community» Needs work

This looks like a good change, but if we do this I think we should also rename the query string to drupal_asset_query_string or similar.

@catch do you mean replace $query_string with $drupal_asset_query_string and $query_string_separator with $drupal_asset_query_string_separator?

$query_string = variable_get('css_js_query_string', '0');
should we use config here instead of variable_get?

@sidharthap
Yes, config should be used instead of variable_get()
See also: https://drupal.org/node/2183531

I mean changing the key used in state.

+++ b/core/includes/theme.inc
@@ -1395,30 +1395,47 @@ function theme_get_setting($setting_name, $theme = NULL) {
+        $query_string = \Drupal::state()->get('system.css_js_query_string') ?: '0';

Like here.

Status:Needs work» Needs review
StatusFileSize
new5.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch clear_cache_logo_favicon-1934508-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks @catch
Corrected the $query_string key.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Novice, -php-novice

The change between #42 and #51 looks fine.

Issue summary:View changes

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 51: clear_cache_logo_favicon-1934508-51.patch, failed testing.

StatusFileSize
new5.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,676 pass(es), 35 fail(s), and 22,337 exception(s).
[ View ]

This needed a re-roll, due to #2210197: Remove public access to Extension::$type, ::$name, ::$filename.

Which requires us to convert from dirname($theme_object->filename) to $theme_object->getPath().

StatusFileSize
new1.11 KB

Forgot to attach an interdiff between 51 and 55, here it is.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 55: clear_cache_logo_favicon-1934508-55.patch, failed testing.

I don't understand enough about core testing procedures to grok why there were so many seemingly unrelated fails. Anyone have any ideas?

Status:Needs work» Needs review
StatusFileSize
new5.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,646 pass(es).
[ View ]
new1.3 KB

$theme_object->getPath should be $theme_object->getPath(), plus I fixed a minor coding standards issue.

Bah. thank you! AND sorry :-|

Status:Needs review» Reviewed & tested by the community

This looks good, patch results are green, and it contains the feedback from #46, back to RTBC.

Issue summary:View changes

Status:Reviewed & tested by the community» Needs work

@catch so

+++ b/core/includes/theme.inc
@@ -887,30 +887,47 @@ function theme_get_setting($setting_name, $theme = NULL) {
+        $query_string = \Drupal::state()->get('system.drupal_asset_query_string') ?: '0';

So this string is not altered on cache clear so this change will not work. We should be testing this.

I guess that @catch meant rename all instances of system.css_js_query_string