Recommend commit message:
Issue #1934508 by jwilson3, clemens.tolboom, wesleydv, matglas86, longwave, sidharthap, superspring, 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
No visible changes out of the box. But, because we've added query strings to the logo and to the favicon URLs, a net positive effect on rendering will occur when a site administrator updates the favicon or logo files on the server. A subsequent cache clear will guarantee all visitors will see the new file instead of the old file from cache.
API changes
The Drupal CSS/JS query-string internal variable has been renamed from css_js_query_string
to drupal_asset_query_string
, and the internal function _drupal_flush_css_js()
to _drupal_flush_assets()
, because both affect not only CSS and JS anymore.
Comment | File | Size | Author |
---|---|---|---|
#91 | interdiff.txt | 735 bytes | joelpittet |
#91 | cache_clear_doesn_t-1934508-91.patch | 14.98 KB | joelpittet |
#89 | interdiff-kinda.txt | 20.06 KB | joelpittet |
#89 | cache_clear_doesn_t-1934508-89.patch | 14.97 KB | joelpittet |
| |||
#83 | cache_clear_doesn_t-1934508-82.patch | 14.95 KB | clemens.tolboom |
Comments
Comment #1
jwilson3Comment #2
jwilson3this should be an assignment operator not a concatenation.
fixed in this patch.
Comment #3
jwilson3Companion patch for d7.
Comment #5
richardj CreditAttribution: richardj commentedThe 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.
Comment #6
richardj CreditAttribution: richardj commentedpatch in #2 works locally and as intended. See #5 for why test may fail.
Comment #7
MegaChriz CreditAttribution: MegaChriz commentedIn 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.
Comment #8
jwilson3Thanks 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).
Comment #9
clemens.tolboomAdded 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.
Comment #10
richardj CreditAttribution: richardj commentedYou 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.
Comment #11
clemens.tolboom@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 :)
Comment #12
richardj CreditAttribution: richardj commented@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!
Comment #13
xjm#7: drupal-cache-controlled-logo-and-favicon-1934508-7.patch queued for re-testing.
Comment #14
xjmWe should use
config()
instead ofvariable_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.)
Comment #15
xjmAlso, while this is a great change, I think it's more of a feature request.
Comment #16
richardj CreditAttribution: richardj commentedI 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.
Comment #17
xjmSee: #1798738: Move css_js_query_string to state system.
Comment #18
jwilson3Agree 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?
Comment #19
matglas86 CreditAttribution: matglas86 commentedI replace the variable_get with the state()-get() as suggested in #17.
Comment #20
matglas86 CreditAttribution: matglas86 commentedOoh I forgot to set it to needs review.
Comment #22
xjmFair enough. Thanks!
Comment #23
matglas86 CreditAttribution: matglas86 commentedI 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.
Comment #24
MathieuSpil CreditAttribution: MathieuSpil commented#23: 1934508-favicon-23.patch queued for re-testing.
Comment #26
jwilson3Edit: Removed useless rant ;O
Comment #27
jwilson3#1798738: Move css_js_query_string to state system is in now.
Comment #28
jwilson3Comment #29
jwilson3patch in #23 needs a reroll.
Comment #30
wesleydv CreditAttribution: wesleydv commentedReroll against head
Comment #31
wesleydv CreditAttribution: wesleydv commentedComment #33
wesleydv CreditAttribution: wesleydv commentedFixing issue from patch #30
Comment #35
wesleydv CreditAttribution: wesleydv commentedFixing issue in ThemeTest.php
Comment #36
jwilson3Sorry for this nitpick, but:
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.
Comment #37
wesleydv CreditAttribution: wesleydv commentedFixed typo described in #36
Comment #38
jwilson3Thanks for that fix.
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".Code comments should use the hyphenated text "query-string".
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.
Comment #39
jwilson3If 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.
Comment #40
jwilson3Comment #41
jwilson3Tagging in case anyone wants to grab this and roll in #39.
Comment #42
superspring CreditAttribution: superspring commentedThis is taking the patch from #38 and applying the review in #39
Comment #42.0
superspring CreditAttribution: superspring commentedClarified first paragraph
Comment #43
rodrigoaguileraTested that it has the correct behaviour and implements the last remarks
Comment #44
jwilson3I've cleaned up the issue summary.
Comment #45
jwilson3Comment #46
catchThis 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.
Comment #47
drupalninja99 CreditAttribution: drupalninja99 commented@catch do you mean replace $query_string with $drupal_asset_query_string and $query_string_separator with $drupal_asset_query_string_separator?
Comment #48
sidharthap$query_string = variable_get('css_js_query_string', '0');
should we use config here instead of variable_get?
Comment #49
MegaChriz CreditAttribution: MegaChriz commented@sidharthap
Yes, config should be used instead of
variable_get()
See also: https://drupal.org/node/2183531
Comment #50
catchI mean changing the key used in state.
Like here.
Comment #51
sidharthapThanks @catch
Corrected the $query_string key.
Comment #52
jwilson3The change between #42 and #51 looks fine.
Comment #53
jwilson3Comment #55
jwilson3This 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()
.Comment #56
jwilson3Forgot to attach an interdiff between 51 and 55, here it is.
Comment #57
longwaveComment #59
jwilson3I don't understand enough about core testing procedures to grok why there were so many seemingly unrelated fails. Anyone have any ideas?
Comment #60
longwave$theme_object->getPath should be $theme_object->getPath(), plus I fixed a minor coding standards issue.
Comment #61
jwilson3Bah. thank you! AND sorry :-|
Comment #62
jwilson3This looks good, patch results are green, and it contains the feedback from #46, back to RTBC.
Comment #63
jwilson3Comment #64
alexpott@catch so
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
Comment #65
jwilson3I've renamed system.drupal_asset_query_string everywhere and added a test for
_drupal_flush_css_js()
to catch the case mentioned in #64. Presumably we should *not* rename the function to _drupal_flush_assets(), but perhaps it does make sense to update the docblock for that function to mention logo and favicon will also be affected, in addition to css and js. Thoughts?Comment #66
jwilson3grammar :( fixed in this follow up.
Comment #69
jwilson3There is something wrong with the test I added, but I don't know what it is. Need a second set of eyeballs on this.
Comment #70
jwilson3I was finally able to figure out that problem above boiled down to the use of REQUEST_TIME for calculating the query string. During a test run, a non-existent query string is first created and then reset within the context of one single request, which was having the effect of recalculating the query string based on the same REQUEST_TIME, hence the query string did not seem to change,.
The solution was to base the query string on the current unix timestamp
time()
instead of REQUEST_TIME. This solved the problem, and tests now pass (locally for me). Lets see what test bot says...Comment #71
jwilson3Heh. I had commented out other tests to speed up the testing process. Here they are back to normal.
Comment #72
clemens.tolboomThe try-catch is weird and was not documented.
It was found through #23 @matglas86 ... is that still needed?
As
StateInterface::get()
has a default I removed the ?: construct.Comment #75
clemens.tolboomRunning the failure on #72 I ran into #2306649: Running phpunit tests via the UI results in a fatal when running the failing test locally.
Applying the patch from #2306649-1: Running phpunit tests via the UI results in a fatal made
Drupal\Tests\Core\Asset\CssCollectionRendererUnitTest
run like a charm locally.So not sure why it failed.
Comment #77
clemens.tolboomHmmm ... the fix is changing the mock returnung '0' instead of 'NULL'.
Running the test through the Simpletest UI changes the conditions as next check behaves differently as running through Simpletest make the core function available instead.
Hope I understood this correctly.
Comment #80
clemens.tolboomPatch needed a rebase. Thanks for the reminder.
Comment #82
clemens.tolboomI ran
sudo -u _www php run-tests.sh --url http://drupal.d8 --class "Drupal\system\Tests\Common\JavaScriptTest,Drupal\Tests\Core\Asset\CssCollectionRendererUnitTest,Drupal\system\Tests\Common\CascadingStylesheetsTest,Drupal\system\Tests\System\ThemeTest"
which failed on the last. There was a missed reroll _drupal_flush_css_js(); which should have been _drupal_flush_assets(); ... weird the testbot is not more informative.
Comment #83
clemens.tolboomComment #84
jwilson3Looks good, passes tests, and passed manual test on http://s60c825e9d4ac265.s2.simplytest.me/.
Initial install has query-string on the druplicon logo as
?0
. Clicking Performance > Clear All Caches, once sets the query string to?nanc2n
on the logo. clearing the caches a second time set the query-string to?nakc3f
.RTBC.
Comment #85
jwilson3Hiding old patches.
Comment #86
jwilson3Hiding the rest of the old patches, updating commit string.
Comment #87
jwilson3Comment #88
alexpottAlso the comment is not wrapped correctly and the last sentence is not grammatically correct. Maybe
We need to catch WhateverException since theme_get_setting() is used during the installer before the state service is available.
What is the specific exception we want to catch here? A general catch seems unwise.
Considering we are touching all of these lines let's simplify this code by passing in the default value. This was not available when this was written.
Comment #89
joelpittetTriage treasure hunting revival. Bumping to 8.1 as it's a feature request.
Re-rolled and did #88.3. See if this even works, hopefully I did ok on re-roll and didn't lose anything in translation.
Comment #91
joelpittetTypos in the last patch.
Comment #93
joelpittetBoth failures are now just ColorTest fails, anybody else want to pick this up?