[Asset] Add CachedVersionStrategy to decorate any Asset version strategy #36371

GromNaN · Apr 6, 2020

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	#35762 (review)
License	MIT
Doc PR	WIP

Easily add a cache decorator ahead of any VersionStrategy. The goal it to cache HTTP requests to a remote JSON manifest.

framework:
    assets:
        json_manifest_path: 'https://cdn.example.com/manifest.json'
        cache_version: true

Issues:

We can a distinct cache item for each asset url that is generated. Caching the contents of the manifest would be a better idea.
If the method applyVersion calls getVersion (like the StaticVersionStrategy does), we get 2 cache items for a single path.
I don't find the best configuration to pass cache options (beta, expiration).

GromNaN · Apr 6, 2020

Hello @nicolas-grekas. I'm not sure this is what you expected when you mentioned a "generic CachedManifestVersionStrategy".
This as the benefit of being compatible with any implementation of VersionStrategyInterface. But it makes a call to the cache backend for every asset url that is generated, whereas the manifest is a file that fit in a single cache item.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

nicolas-grekas · Apr 19, 2020

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -585,6 +585,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
                        ->scalarNode('version')->defaultNull()->end()
                        ->scalarNode('version_format')->defaultValue('%%s?%%s')->end()
                        ->scalarNode('json_manifest_path')->defaultNull()->end()
+                        ->booleanNode('cache_version')->defaultFalse()->end()


why not defaultNull?

I've kept the boolean and removed options.

use_cache => true/false/string => when string, it's the name of the cache pool to use?

(or just cache btw)

I still don't get the name cache_version
cache: true/false looks better to me. Do I miss something?

I've renamed to cache.
I wanted to avoid confusion. This cache the result of the URL generation, a developer might think this is the HTTP cache when the browser requests the URL.

cache_json_manifest then?

or json_manifest_cache might even be better?

It can cache any version strategy, not only the manifest.
We could also remove this option and always enable the cache on remote json manifests?

Can we? How would this work with env vars?
Or we might just always enable the cache, even for local files.
For thoughts :)

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Asset/VersionStrategy/CachedVersionStrategy.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml

GromNaN

I've applied all the suggestions.

what about removing the options altogether and deegating them to the cache pool instead?
using ProxyAdapter, this is quite easy to achieve

How would you insert the decorator between the cache.assets service and its injection into CachedVersionStrategy ?

GromNaN · Apr 20, 2020

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -585,6 +585,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
                        ->scalarNode('version')->defaultNull()->end()
                        ->scalarNode('version_format')->defaultValue('%%s?%%s')->end()
                        ->scalarNode('json_manifest_path')->defaultNull()->end()
+                        ->booleanNode('cache_version')->defaultFalse()->end()


I've kept the boolean and removed options.

src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml

nicolas-grekas · Apr 21, 2020

How would you insert the decorator between the cache.assets service and its injection into CachedVersionStrategy ?

nothing to do here: ppl will know how to redefine the cache.assets service and redefine a dedicated pool if you follow my last comment.

GromNaN · Apr 23, 2020

Nice, that something I'll try to describe in the doc.

nicolas-grekas · May 4, 2020

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -585,6 +585,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
                        ->scalarNode('version')->defaultNull()->end()
                        ->scalarNode('version_format')->defaultValue('%%s?%%s')->end()
                        ->scalarNode('json_manifest_path')->defaultNull()->end()
+                        ->booleanNode('cache_version')->defaultFalse()->end()


I still don't get the name cache_version
cache: true/false looks better to me. Do I miss something?

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

framework: assets: json_manifest_path: 'https://cdn.example.com/manifest.json' cache_version: true

GromNaN · May 19, 2020

I'd rather use cache on top of the HttpClient response (see #36858), instead of caching every file version generated by the version strategy.
Closing this PR.

carsonbot added Status: Needs Review Asset Feature labels Apr 6, 2020

GromNaN force-pushed the cached-version-strategy branch from 7aa072a to 6c779c7 Compare April 6, 2020 22:26

nicolas-grekas added this to the next milestone Apr 6, 2020

GromNaN force-pushed the cached-version-strategy branch 2 times, most recently from ae46adb to 05e694a Compare April 12, 2020 21:51

GromNaN marked this pull request as ready for review April 12, 2020 22:03

nicolas-grekas reviewed Apr 19, 2020

View reviewed changes

GromNaN force-pushed the cached-version-strategy branch 2 times, most recently from 1acd5a3 to 7a43c89 Compare April 20, 2020 21:27

GromNaN commented Apr 20, 2020

View reviewed changes

nicolas-grekas reviewed May 4, 2020

View reviewed changes

Add CachedVersionStrategy to decorate any Asset version strategy

dc90706

framework: assets: json_manifest_path: 'https://cdn.example.com/manifest.json' cache_version: true

GromNaN force-pushed the cached-version-strategy branch 2 times, most recently from 573bb34 to fd5a420 Compare May 6, 2020 09:40

Rename cache_version to cache

7182255

GromNaN force-pushed the cached-version-strategy branch from fd5a420 to 7182255 Compare May 6, 2020 09:55

GromNaN closed this May 19, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

GromNaN deleted the cached-version-strategy branch October 20, 2021 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Asset] Add CachedVersionStrategy to decorate any Asset version strategy #36371

[Asset] Add CachedVersionStrategy to decorate any Asset version strategy #36371

GromNaN commented Apr 6, 2020 •

edited

Loading

GromNaN commented Apr 6, 2020

nicolas-grekas Apr 19, 2020

GromNaN Apr 20, 2020

nicolas-grekas Apr 21, 2020

nicolas-grekas Apr 21, 2020

nicolas-grekas May 4, 2020

GromNaN May 6, 2020

nicolas-grekas May 6, 2020

nicolas-grekas May 6, 2020

GromNaN May 6, 2020

nicolas-grekas May 6, 2020

GromNaN left a comment

GromNaN Apr 20, 2020

nicolas-grekas commented Apr 21, 2020

GromNaN commented Apr 23, 2020

nicolas-grekas May 4, 2020

GromNaN commented May 19, 2020 •

edited

Loading

Search code, repositories, users, issues, pull requests...

[Asset] Add CachedVersionStrategy to decorate any Asset version strategy #36371

[Asset] Add CachedVersionStrategy to decorate any Asset version strategy #36371

Conversation

GromNaN commented Apr 6, 2020 • edited Loading

GromNaN commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 21, 2020

GromNaN commented Apr 23, 2020

Choose a reason for hiding this comment

GromNaN commented May 19, 2020 • edited Loading

GromNaN commented Apr 6, 2020 •

edited

Loading

GromNaN commented May 19, 2020 •

edited

Loading