Respect enable_asset_timestamp settings for pipelined Assets

[Sommerregen]

I don't know if this is intentional, but the enabled enable_asset_timestamp settings is not respected for pipelined assets. Even if I disable this setting the cache key is always added.

I know why the cache key should be present due to browser cache invalidation of changes. However what I don't understand is then, why this setting currently only affects non-pipelined assets (see e.g. Assets.php#L556).

This PR is for the lazy-man. Instead of opening an issue we can discuss it here or if this behavior is intentional, feel free to close it.

[flaviocopes]

I think it's simply missing in that case, no real reason behind that.

But it should also take in consideration the enable_asset_timestamp: false case, right?

[Sommerregen]

@flaviocopes That's a good point! I'll wait for @rhukster maybe he can shed some more light on it. Concerning the enable_asset_timestamp: false case, it runs as follows: $this->timestamp is '' as this is the default (L80) in (L180) it gets set to the Grav cache key.

[rhukster]

Here's the thing.. The timestamp was intended for regular assets links only. This is because if you have LONG timeouts or expires for style/js assets, the browser will not even ask for refreshed files unless the url changes somehow. A timestamp is enough to trigger this.

For the pipeline, once created, the pipeline is not updated or regenerated no matter if a CSS/JS file is modified. So really the only way to force the regeneration is to clear the asset cache. However on regeneration, the filename is not going to change as it's a named based on md5 of the files involved. So even after regeneration, it might be cached or not even requested, hence the forced timestamp.

Also, changing this behavior might have a negative effect on the behavior of existing sites.

However, i have an idea for a solution:

1. We add a timestamp to the name generation process. ($uid) in both pipelineCss and pipelineJs
2. We add the check for timestamp toggle as proposed.

This will ensure that every time the css/pipeline is regenerated, the name is unique and thereforce not going to be cached. The timestamp at the end will not really have much of an effect, but added merely for consistency and expected results.

This probably will also fix the problems we see with CDN caching and not updating with just a bin/grav clear --assets-only, requiring a full bin/grav clear --all to force a cache key change, and the CDN picking that up.

[Sommerregen]

Ok, I fully understand. Wasn't aware that the hash is based on the files involved. Thought it was somehow also derived from Grav cache key... But you are right (L677) and I really like your idea! :-D I have updated my PR. Hope you like it.

[rhukster]

Actually I don't think it should be the cache key, rather an actual time stamp of now

[rhukster]

looks good to me!

[Sommerregen]

After a good sleep, I don't think that having such a timestamp with time() is stable enough not to build the pipeline any time again a user requests a page. I think we should gather all modified times in the addJs and addCss methods. For local assets that's easy and for remote ones providing a default (a constant, like 0).

[rhukster]

Actually you are right, this won't work correctly because the file will be generated every time. We need a last generated timestamp that is actually stored in the assets/ folder I think. Then we can reuse this timestamp to build the filename when looking to see if the file exists.

[rhukster]

i've switched it back to cache key for the time being so it will at least not recreate the pipeline everytime.