Allow asset cache busting if not combining assets #1404
Conversation
|
I'm a little worried this is not entirely correct. First of all, the time might already be passed to the combiner, we should use that value then. Also, paths might be absolute/external, not sure we would need to handle that case here? Honestly, I think we should rather move away from the combiner if we don't combine scripts... |
We do.
That's not related to this PR. I don't want to touch existing logic. This is already the case, I just added a version flag, that's all. |
Which is however enabled by default and likely to be removed in a future version, because we came to the conclusion that combining scripts also makes sense with HTTP/2. How does the Symfony asset component handle this? |
I disagree with this. If you have a page where layout files change often, it does make sense to keep them separately. If you want to cover all use cases there's only one way: The admins have to be able to create bundles themselves and specify stuff like preload/prefetch etc. on their own for every bundle. However, I don't really think it's Contao's task to have support for pre-processing, bundling, optimizing, minifying etc. files because there are so many front end packaging tools like webpack, browserify, brunch, make, parcel, rollup and what not for this. They will always do that better than we ever will. We should focus on managing content, not front end tooling. That's definitely something we should all discuss about to find a general solution for the future but for now, this PR totally makes sense to me. |
We agreed that there are situations where this still makes sense in HTTP/2. The problem is that is should be possible to create multiple bundles based on files that are always used on the same pages. But Contao does not offer that, so with HTTP/2 it could be better to not combine instead of create a different combination on each page essentially causing multiple downloads of the same stuff for no reason. I do like cache busting very much, but I don't use Contao for that anymore. Because I don't trust it for some reason, maybe because it did not work for me for years. Using Symfony Encore creates files with random names so cache busting always works correctly. I've been using it in multiple projects by now, I think we should come up with a future solution using it and leave the existing one for legacy support and "non-optimized" setups. |
|
Symfony Encore might be worth a shot. 👍 |
|
I agree. We might just generally thing about better support/integration of other tools that are better suited for all of this. For for now, we still have this checkbox and therefore that PR makes sense to me. |
I'm already using and loving it :) |
|
One issue I have with this PR is that it will not provide cache busting for scripts that are added elsewhere via |
|
Can you add it? |
|
Hm, well, where should it be done? It could be done within Otherwise it would have to be done manually in various places (4 occurrences in |
Then probably there, right? |
|
The backend should also use cache busting, imho. Every once in a while errors like this happens and then we have to tell customers to clear the cache - which they always react a little disgruntled, because they do not see why they would have to do it and see it as a flaw of the web application. |
|
As discussed in Mumble on March 15th, the PR is incomplete. It should also handle the But 👍 for the feature in general. |
|
Alright, will work on a solution then 👍 |
|
There you go.
BTW, since Contao 4.5 this is already fixed for all the assets being supplied by the Core itself. Only |
|
@discordier also suggested not to check the mtime in production at all (use the file if its there). Does this make sense to you, too? |
|
No. Can you elaborate? |
|
We are now checking the mtime to determine whether a file needs to be recompiled in development and production. However, similar to the Symfony cache, we should probably only check and recompile in development and not in production. |
|
Why should it be only done in development? Doing so would make cache busting pointless. |
|
Yeah we could add hundreds of optimizations but I won't do them in legacy code. If we find an alternative for these horrible global arrays I'll happily think about efficiency :) |
The Symfony cache is not "busted" in production, either. If you change something, you have to clear it. This saves hundreds of |
|
Oh I see what you mean now. |
Weird. But our customers are still affected by the aforementioned bug, if they do not clear their browser cache. // edit: oh, it is not implemented in Contao 4.4. |
Yeah, it's not a bug. It's a feature :) |
| { | ||
| if (file_exists(TL_ROOT . '/' . $href)) |
There was a problem hiding this comment.
If $mtime is not null, we do not need this file_exist() check, do we?
if ($mtime === null && file_exists(TL_ROOT . '/' . $href))
{
$mtime = filemtime(TL_ROOT . '/' . $href);
}
|
It seems that the suffix is added twice if the asset URL comes from the Symfony asset component: <link rel="stylesheet" href="bundles/contaocalendar/calendar.min.css?324ac593?d41d8cd9"> |
|
Also it does not handle external resources: <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Open+Sans?d41d8cd9"> |
|
And last but not least we should not apply it to combined assets: <script src="assets/js/jquery.min.js-f15f81ff.js?ec56bbf4"></script>The combined file URL already contains |
|
I cannot reproduce any of these cases. The problem is that there's no central routine and it happens at many places. Do you fixing the cases you discovered yourself directly? |
|
This is how it works for me: --- a/src/Resources/contao/classes/BackendTemplate.php
+++ b/src/Resources/contao/classes/BackendTemplate.php
@@ -78,7 +78,7 @@ class BackendTemplate extends \Template
}
else
{
- $strStyleSheets .= \Template::generateStyleTag($this->addStaticUrlTo($stylesheet), $options->media);
+ $strStyleSheets .= \Template::generateStyleTag($this->addStaticUrlTo($stylesheet), $options->media, $options->mtime);
}
}
@@ -107,7 +107,7 @@ class BackendTemplate extends \Template
}
else
{
- $strJavaScripts .= \Template::generateScriptTag($this->addStaticUrlTo($javascript), $options->async);
+ $strJavaScripts .= \Template::generateScriptTag($this->addStaticUrlTo($javascript), $options->async, $options->mtime);
}
}
--- a/src/Resources/contao/library/Contao/Combiner.php
+++ b/src/Resources/contao/library/Contao/Combiner.php
@@ -269,14 +269,23 @@ class Combiner extends \System
{
$return = $this->getFileUrls();
- if ($this->strMode == self::JS)
+ foreach ($return as $k=>$v)
{
- return implode('"></script><script src="', $return);
+ $return[$k] = str_replace('|', '" media="', $v);
+
+ if (file_exists(TL_ROOT . '/' . $v))
+ {
+ $return[$k] .= '?' . substr(md5(filemtime(TL_ROOT . '/' . $v)), 0, 8);
+ }
+ elseif (file_exists(TL_ROOT . '/' . $this->strWebDir . '/' . $v))
+ {
+ $return[$k] .= '?' . substr(md5(filemtime(TL_ROOT . '/' . $this->strWebDir . '/' . $v)), 0, 8);
+ }
}
- foreach ($return as $k=>$v)
+ if ($this->strMode == self::JS)
{
- $return[$k] = str_replace('|', '" media="', $v);
+ return implode('"></script><script src="', $return);
}
return implode('"><link rel="stylesheet" href="', $return);
--- a/src/Resources/contao/library/Contao/Template.php
+++ b/src/Resources/contao/library/Contao/Template.php
@@ -506,11 +506,36 @@ abstract class Template extends \Controller
*
* @param string $href The script path
* @param string $media The media type string
+ * @param integer $mtime The file mtime
*
* @return string The markup string
*/
- public static function generateStyleTag($href, $media=null)
+ public static function generateStyleTag($href, $media=null, $mtime=null)
{
+ // Add the filemtime if not given and not an external or combined file
+ if ($mtime === null && !preg_match('@(?:^https?://)|(?:-[a-f0-9]{8}\.css$)@', $href))
+ {
+ if (file_exists(TL_ROOT . '/' . $href))
+ {
+ $mtime = filemtime(TL_ROOT . '/' . $href);
+ }
+ else
+ {
+ $webDir = \StringUtil::stripRootDir(\System::getContainer()->getParameter('contao.web_dir'));
+
+ // Handle public bundle resources in web/
+ if (file_exists(TL_ROOT . '/' . $webDir . '/' . $href))
+ {
+ $mtime = filemtime(TL_ROOT . '/' . $webDir . '/' . $href);
+ }
+ }
+ }
+
+ if ($mtime !== null)
+ {
+ $href .= '?' . substr(md5($mtime), 0, 8);
+ }
+
return '<link rel="stylesheet" href="' . $href . '"' . (($media && $media != 'all') ? ' media="' . $media . '"' : '') . '>';
}
@@ -533,11 +558,36 @@ abstract class Template extends \Controller
*
* @param string $src The script path
* @param boolean $async True to add the async attribute
+ * @param integer $mtime The file mtime
*
* @return string The markup string
*/
- public static function generateScriptTag($src, $async=false)
+ public static function generateScriptTag($src, $async=false, $mtime=null)
{
+ // Add the filemtime if not given and not an external or combined file
+ if ($mtime === null && !preg_match('@(?:^https?://)|(?:-[a-f0-9]{8}\.js)@', $src))
+ {
+ if (file_exists(TL_ROOT . '/' . $src))
+ {
+ $mtime = filemtime(TL_ROOT . '/' . $src);
+ }
+ else
+ {
+ $webDir = \StringUtil::stripRootDir(\System::getContainer()->getParameter('contao.web_dir'));
+
+ // Handle public bundle resources in web/
+ if (file_exists(TL_ROOT . '/' . $webDir . '/' . $src))
+ {
+ $mtime = filemtime(TL_ROOT . '/' . $webDir . '/' . $src);
+ }
+ }
+ }
+
+ if ($mtime !== null)
+ {
+ $src .= '?' . substr(md5($mtime), 0, 8);
+ }
+
return '<script src="' . $src . '"' . ($async ? ' async' : '') . '></script>';
}
Note that checking for combined URLs is done via regex, which I think is ugly and error-prone. Maybe we can come up with a better solution? |
|
Also I don't know if we really need to add suffixes in debug mode? If you already access your application via a separate entry point, you might as well open the browser's dev tools. 😄 |
If you do not enable the checkbox to combine scripts in the layout, I think we should add a simple string to the file urls so we can benefit from cache busting. I'll add another PR for the manager-bundle to renable asset caching again for all assets by default.