[Asset] Adding a new version strategy that reads from a manifest JSON file #22046

weaverryan · 2017-03-17T19:25:53Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	symfony/symfony-docs#7659

Hi guys!

Often, when using a frontend task manager or bundler (e.g. webpack of gulp), the final assets are dumped with a version or content hash in the filename itself (e.g. main.123abc.css). To know what the correct, current hashed filename is, you'll dump a manifest.json file - e.g.

{
  "main.js": "main.123abc.js",
  "css/styles.css": "css/styles.555def.css"
}

Examples: gulp-rev and webpack-manifest-plugin.

This PR adds a new version strategy that will look up the asset path (e.g. main.css) in that file and return the final, versioned path. Some people may dump manifest files in other formats, but I think this catches the most common use-case (and you can always still create your own version strategy). I've written this to be "forgiving" - if a path doesn't exist in the manifest, the path is simply returned, unaltered.

Another implementation could have been to add a new Twig filter (e.g. {{ asset('main.css|manifest_path) }}) - but I thought I'd try first using the existing versioning system.

Usage

# app/config/config.yml
framework:
    # ...
    assets:
       # added validation prevents you from setting json_manifest_path AND version, for example
        json_manifest_path: '%kernel.root_dir%/../web/manifest.json'

{# someTemplate.html.twig #}

{# use asset() just like normal #}
<script src="{{ asset('js/main.js') }}"></script>

TODO

fabbot hates my invalid json syntax file... even though I tried to be clever and not give it a .json suffix :)

javiereguiluz

A big 👍 from me!

This is so common in modern setups that Symfony should really provide this asset versioning.

I also agree with being forgiving and returning the same path when it doesn't exist in the manifest file.

javiereguiluz · 2017-03-17T21:32:17Z

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

@@ -589,6 +597,12 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
                                    })
                                    ->thenInvalid('You cannot use both "version_strategy" and "version" at the same time under "assets" packages.')
                                ->end()
+                                ->validate()
+                                    ->ifTrue(function ($v) {
+                                        return isset($v['version_strategy']) && $v['version_strategy'] == 'json_manifest' && !isset($v['manifest_path']);


== 'json_manifest' -> === 'json_manifest'

javiereguiluz · 2017-03-17T21:32:23Z

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

@@ -558,6 +559,12 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
                        })
                        ->thenInvalid('You cannot use both "version_strategy" and "version" at the same time under "assets".')
                    ->end()
+                    ->validate()
+                        ->ifTrue(function ($v) {
+                            return isset($v['version_strategy']) && $v['version_strategy'] == 'json_manifest' && !isset($v['manifest_path']);


== 'json_manifest' -> === 'json_manifest'

javiereguiluz · 2017-03-18T10:44:18Z

src/Symfony/Component/Asset/Tests/fixtures/manifest-invalid-json

+{
+  "main.js": main.123abc.js",
+  "css/styles.css": "css/styles.555def.css"
+}


We should add a new line at the end of this file.

javiereguiluz · 2017-03-18T10:45:51Z

src/Symfony/Component/Asset/Tests/fixtures/manifest-invalid-json

@@ -0,0 +1,4 @@
+{


It's clever to not use .json extension to avoid fabbot issues ... but that could complicate the maintenance of this. Imagine looking at this conde in 1 year and wondering: why this file doesn't have an extension? Is this an error? Should I add the extension now? etc.

So, let's add the .json extension and forget about fabbot issues.

thewilkybarkid · 2017-03-20T08:15:30Z

Looks to replace https://github.com/irozgar/gulp-rev-versions-bundle.

A useful feature would be to compile the manifest into a static array, to avoid reading it at runtime (in production).

stof · 2017-03-21T10:04:41Z

This changes the meaning of the version_strategy configuration, as this is not a service id anymore in some cases.
IMO, the usage of the json manifest strategy should be detected based only on manifest_path being configured and no custom strategy being provided, similar to the way the static version strategy is configured

stof · 2017-03-21T10:05:34Z

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

+    {
+        $manifestPath = $this->getManifestPath($path);
+
+        return $manifestPath ? $manifestPath : $path;


return $manifestPath ?: $path;

stof · 2017-03-21T10:06:05Z

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

+
+    private $manifestData;
+
+    public function __construct($manifestPath)


please add the phpdoc describing the argument as a string, as we don't have a typehint to tell it

stof · 2017-03-21T10:07:22Z

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

+            }
+
+            $this->manifestData = json_decode(file_get_contents($this->manifestPath), true);
+            if (0 < $errorCode = json_last_error()) {


$errorCode is unused

stof · 2017-03-21T10:08:38Z

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

+    {
+        $def = new ChildDefinition('assets.json_manifest_version_strategy');
+        $def
+            ->replaceArgument(0, $manifestPath)


IMO, we should also make this service private, to allow it to be inlined

I agree, but I copied this code from the above method, which is not private. So, might there be a reason for it to be public?

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

Lines 843 to 856 in 4927993

private function createPackageDefinition($basePath, array $baseUrls, Reference $version)

{

if ($basePath && $baseUrls) {

throw new \LogicException('An asset package cannot have base URLs and base paths.');

}

$package = new ChildDefinition($baseUrls ? 'assets.url_package' : 'assets.path_package');

$package

->setPublic(false)

->replaceArgument(0, $baseUrls ?: $basePath)

->replaceArgument(1, $version)

;

return $package;

I'm going to try making it private.

GromNaN · 2017-03-23T07:44:36Z

Awesome feature that I've seen implemented in many projects, this JSON format is so simple that it is always the one that is used.
Is it possible to have many manifests ? I remember having JS, CSS and images assets in different manifests files. But also mobile and desktop assets in different manifests.

Is the solution to define an asset package for each manifest file ?

javiereguiluz · 2017-03-23T07:57:51Z

@GromNaN is having multiple manifest still common? I only know the Webpack solution and they create just 1 manifest.json for all assets(images, CSS, JS).

…m a JSON manifest file

…ing the invalid JSON

(fabbot dislikes invaild JSON)

…s all that's needed to activate This keeps the original behavior of version_strategy: this is always a service id

weaverryan · 2017-03-23T11:19:11Z

PR Updated!

I like Stof's suggestion about keeping version_strategy always as an id. I updated the config usage in the PR description
@thewilkybarkid I don't know if compiling a static array from the JSON is a good idea or not. It might be unexpected that simply updating the file wouldn't cause Symfony to pick up the changes. I'm not sure if the decoding overhead is significant enough to care - decoding a 30 line manifest takes .00011 seconds (.11 milliseconds), though it is a filesystem hit!
@GromNaN That's interesting, I want to make sure we've at least thought through this scenario (though ultimately, the strategy doesn't need to work for everyone. In general, can you think of a solution other than multiple packages? Even if we made this feature much bigger and supported multiple manifest files, in Twig, you still would need to specify which one you need, and the only config you can pass to asset() is the $packageName. Or, the strategy would need to somehow be smart enough to know which manifest file to use for each package, which now sounds like you would/should have a custom version strategy.

This is ready to be reviewed again! Thanks!

javiereguiluz · 2017-03-23T11:28:39Z

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

@@ -582,6 +595,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
                                        ->end()
                                    ->end()
                                    ->scalarNode('version_format')->defaultNull()->end()
+                                    ->scalarNode('json_manifest_path')->defaultNull()->end()


Just asking: would manifest_path be a more future-proof name than json_manifest_path ? Or is this too much future thinking? Can we expect to support other formats different than JSON?

Same for JsonManifestVersionStrategy -> ManifestVersionStrategy ?

It's a good thought. I don't know! It's a fairly easy thing to deprecate and change in the future anyways, and I can't see any other format in the near future. So let's go with what we like best right now. I changed to json_manifest_path because I thought it might be more clear what this manifest thing should be.

lgtm as json*

… way is in 2 method calls

GromNaN · 2017-03-23T13:25:01Z

@javiereguiluz @weaverryan My actual use case is the following:
Having a website on www.example.com with an mirror mobile site m.example.com
Routing and controllers are the same for both domains, but depending on the domain the view listener render a different template (thanks to Twig namespaces).
Both websites are very different in terms of design, that why the frontend developers chose to have 2 separate builds, generating 2 separate JSON manifests. The "mobile" templates use "mobile" assets while the "desktop" templates use "desktop" assets.

Regarding the usage of a PHP file to cache the content of the JSON into a PHP array, from my experience on high traffic websites there is no performance issue at reading the same JSON file on each request: the OS caches the file content in memory and native JSON parsing is very fast.

weaverryan · 2017-03-23T13:36:21Z

@GromNaN Hmm, in that case, it would be most convenient for the listener to be able to change the manifest file. If you created a default package and a mobile package, then in the listener, if you're on mobile, you could make the mobile package the default:

// in the listener... if mobile
// using the container for clarity
$packages = $this->container->get('assets.packages');
$packages->setDefaultPackage($packages->getPackage('mobile'));

And then your templates wouldn't need to specify a package everywhere. Of course, since they're using different templates, they could just specify the mobile package when using asset() instead of the above code.

I think this is actually a pretty decent solution. In other words, I think this PR solves the 99%, and makes the 1% reasonably easy.

Thanks for the reply :)

fabpot · 2017-03-23T16:24:50Z

@weaverryan For the invalid JSON file, just add it to .php_cs.dist

fabpot · 2017-03-23T16:25:07Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

@@ -4,6 +4,8 @@ CHANGELOG
 3.3.0
 -----

+ * A new version strategy option called json_manifest_path was added


added a new ...

fabpot · 2017-03-23T16:26:40Z

src/Symfony/Component/Asset/CHANGELOG.md

@@ -1,6 +1,11 @@
 CHANGELOG
 =========

+3.3.0
+-----
+ * JsonManifestVersionStrategy was added as a way to read final,


added Json...

fabpot · 2017-03-23T16:27:15Z

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

+ *         "css/styles.css": "css/styles.555abc.css"
+ *     }
+ *
+ * You could then as for the version of "main.js" or "css/styles.css".


fabpot · 2017-03-23T16:27:24Z

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

+class JsonManifestVersionStrategy implements VersionStrategyInterface
+{
+    private $manifestPath;
+


empty line can be removed

fabpot · 2017-03-23T16:27:32Z

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

+    private $manifestData;
+
+    /**
+     * @param string $manifestPath Absolute path to the manifest file.


no dot at the end

fabpot · 2017-03-23T16:27:48Z

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

+     *
+     * @param string $path
+     *
+     * @return string


both @param and @return can be removed

GromNaN · 2017-03-23T16:49:17Z

The manifest could be a remote file accessible over HTTP ; that appends when assets and the app are deployed on distinct servers asynchronously. In that case, it is necessary to use an HTTP client (other that file_get_contents) and a cache layer.
This feature can be added once this PR is merged.

fabpot · 2017-03-23T16:58:37Z

@GromNaN I would say that we don't want to support that use case.

fabpot · 2017-03-23T18:25:29Z

👍

weaverryan · 2017-03-23T18:27:12Z

@GromNaN I thought about that use-case, it's also relevant with webpack-dev-server, but ultimately, I didn't want it, at least for now: the user should make sure there's a physical JSON file - it could be part of their deploy process (or, they can of course add their own strategy!)

Tweaks made! But fabbot and I are still fighting - is the JSON check done through PhpCsFixer? Or did I just not get the config right in .php_cs.dist?

weaverryan · 2017-03-23T18:30:57Z

@fabpot if my silly invalid JSON file is going to be a problem with fabbot, let me know - I could re-work things - i.e. save a tmp invalid JSON file in the test and use that (so no committed invalid JSON file).

fabpot · 2017-03-23T18:31:43Z

@weaverryan Don't worry about that. I will see why it's not taken into account by fabbot. Keep the code as is.

nicolas-grekas

👍 with optional comments

nicolas-grekas · 2017-03-23T18:44:16Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

@@ -4,6 +4,8 @@ CHANGELOG
 3.3.0
 -----

+ * Added a new new version strategy option called json_manifest_path


quotes around "json_manifest_path"?

nicolas-grekas · 2017-03-23T18:44:30Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

@@ -4,6 +4,8 @@ CHANGELOG
 3.3.0
 -----

+ * Added a new new version strategy option called json_manifest_path
+   that allows you to use the JsonManifestVersionStrategy.


backticks here?

nicolas-grekas · 2017-03-23T18:45:22Z

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

@@ -582,6 +595,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
                                        ->end()
                                    ->end()
                                    ->scalarNode('version_format')->defaultNull()->end()
+                                    ->scalarNode('json_manifest_path')->defaultNull()->end()


lgtm as json*

nicolas-grekas · 2017-03-23T18:48:05Z

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

+        if (null !== $jsonManifestPath) {
+            $def = new ChildDefinition('assets.json_manifest_version_strategy');
+            $def->replaceArgument(0, $jsonManifestPath);
+            $def->setPublic(false);


move to xml?

nicolas-grekas · 2017-03-23T18:50:42Z

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

+    {
+        $manifestPath = $this->getManifestPath($path);
+
+        return $manifestPath ?: $path;


could be a oneliner :)

weaverryan · 2017-03-24T01:06:16Z

Minor comments addressed!

fabpot · 2017-03-25T16:23:13Z

Thank you @weaverryan.

…tion (robfrawley) This PR was submitted for the master branch but it was merged into the 3.3 branch instead (closes #8004). Discussion ---------- [WIP] Document "framework.assets.json_manifest_path" option This pull request adds initial documentation for the newly introduced asset JSON manifest file path option: `framework.assets.json_manifest_path`. Resolves #7659. For reference on this new configuration option, see symfony/symfony#22046 and [symfony.com/blog/new-in-symfony-3-3-manifest-based-asset-versioning](https://symfony.com/blog/new-in-symfony-3-3-manifest-based-asset-versioning). Commits ------- 13a3070 document new framework.assets.json_manifest_path option

carsonbot added Status: Needs Review Asset Feature labels Mar 17, 2017

javiereguiluz approved these changes Mar 17, 2017

View reviewed changes

javiereguiluz reviewed Mar 18, 2017

View reviewed changes

javiereguiluz mentioned this pull request Mar 20, 2017

Added a new asset versioning strategy that reads a manifest JSON file symfony/symfony-docs#7659

Closed

stof requested changes Mar 21, 2017

View reviewed changes

javiereguiluz added this to the 3.3 milestone Mar 23, 2017

weaverryan added 9 commits March 23, 2017 06:52

[Asset][FrameworkBundle] Adding a new version strategy that reads fro…

e41cf49

…m a JSON manifest file

fabbot

1a3d465

Removing .json from extension of a file to work around fabbot not lik…

6d19549

…ing the invalid JSON

Fixing a test

36fa8b3

== => ===

0a99143

Another attempt at faking out fabbot. Quick fabbot - look over there!

b2c7d32

(fabbot dislikes invaild JSON)

Line breaks and not being clever with invalid - just ending with .json

9ed9c57

Tweaks thanks to Stof!

aa32896

Reworking feature so that json_manifest_path is the config key, and i…

99251c3

…s all that's needed to activate This keeps the original behavior of version_strategy: this is always a service id

weaverryan force-pushed the asset_manifest_path branch from 7a652ae to 99251c3 Compare March 23, 2017 10:53

weaverryan added 2 commits March 23, 2017 07:08

Thanks fabbot!

bebf674

Making test compatible with older phpunit versions

5955f17

javiereguiluz reviewed Mar 23, 2017

View reviewed changes

Fixing test - the old way is all inside setExpectedException, the new…

14d50e1

… way is in 2 method calls

fabpot reviewed Mar 23, 2017

View reviewed changes

Tweaks thanks to fabpot

ff8869a

nicolas-grekas approved these changes Mar 23, 2017

View reviewed changes

Very minor changes thanks to nicolas-grekas

ada2471

fabpot closed this in 8901c7e Mar 25, 2017

weaverryan deleted the asset_manifest_path branch March 25, 2017 17:22

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

robfrawley mentioned this pull request Jun 7, 2017

[WIP] Document "framework.assets.json_manifest_path" option symfony/symfony-docs#8004

Closed


		private $manifestData;

		public function __construct($manifestPath)

	private function createPackageDefinition($basePath, array $baseUrls, Reference $version)
	{
	if ($basePath && $baseUrls) {
	throw new \LogicException('An asset package cannot have base URLs and base paths.');
	}

	$package = new ChildDefinition($baseUrls ? 'assets.url_package' : 'assets.path_package');
	$package
	->setPublic(false)
	->replaceArgument(0, $baseUrls ?: $basePath)
	->replaceArgument(1, $version)
	;

	return $package;

[Asset] Adding a new version strategy that reads from a manifest JSON file #22046

[Asset] Adding a new version strategy that reads from a manifest JSON file #22046

Conversation

weaverryan commented Mar 17, 2017 • edited Loading

Usage

TODO

javiereguiluz left a 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

thewilkybarkid commented Mar 20, 2017 • edited Loading

stof commented Mar 21, 2017

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 commented Mar 23, 2017

javiereguiluz commented Mar 23, 2017

weaverryan commented Mar 23, 2017

javiereguiluz Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN commented Mar 23, 2017 • edited Loading

weaverryan commented Mar 23, 2017

fabpot commented Mar 23, 2017

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 commented Mar 23, 2017 • edited Loading

fabpot commented Mar 23, 2017

fabpot commented Mar 23, 2017

weaverryan commented Mar 23, 2017

weaverryan commented Mar 23, 2017

fabpot commented Mar 23, 2017

nicolas-grekas left a 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

weaverryan commented Mar 24, 2017

fabpot commented Mar 25, 2017

weaverryan commented Mar 17, 2017 •

edited

Loading

thewilkybarkid commented Mar 20, 2017 •

edited

Loading

javiereguiluz Mar 23, 2017 •

edited

Loading

GromNaN commented Mar 23, 2017 •

edited

Loading

GromNaN commented Mar 23, 2017 •

edited

Loading