8000 [Asset] Adding a new version strategy that reads from a manifest JSON file by weaverryan · Pull Request #22046 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ CHANGELOG
3.3.0
-----

* A new version strategy option called json_manifest_path was added
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a new ...

that allows you to use the JsonManifestVersionStrategy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backticks here?

* Added support for the `controller.service_arguments` tag, for injecting services into controllers' actions
* Deprecated `cache:clear` with warmup (always call it with `--no-warmup`)
* Deprecated the "framework.trusted_proxies" configuration option and the corresponding "kernel.trusted_proxies" parameter
Expand Down
8000
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
->scalarNode('version_strategy')->defaultNull()->end()
->scalarNode('version')->defaultNull()->end()
->scalarNode('version_format')->defaultValue('%%s?%%s')->end()
->scalarNode('json_manifest_path')->defaultNull()->end()
->scalarNode('base_path')->defaultValue('')->end()
->arrayNode('base_urls')
->requiresAtLeastOneElement()
Expand All @@ -567,6 +568,18 @@ 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']) && isset($v['json_manifest_path']);
})
->thenInvalid('You cannot use both "version_strategy" and "json_manifest_path" at the same time under "assets".')
->end()
->validate()
->ifTrue(function ($v) {
return isset($v['version']) && isset($v['json_manifest_path']);
})
->thenInvalid('You cannot use both "version" and "json_manifest_path" at the same time under "assets".')
->end()
->fixXmlConfig('package')
->children()
->arrayNode('packages')
Expand All @@ -582,6 +595,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
->end()
->end()
->scalarNode('version_format')->defaultNull()->end()
->scalarNode('json_manifest_path')->defaultNull()->end()
Copy link
Member
@javiereguiluz javiereguiluz Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm as json*

->scalarNode('base_path')->defaultValue('')->end()
->arrayNode('base_urls')
->requiresAtLeastOneElement()
Expand All @@ -598,6 +612,18 @@ 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']) && isset($v['json_manifest_path']);
})
->thenInvalid('You cannot use both "version_strategy" and "json_manifest_path" at the same time under "assets" packages.')
->end()
->validate()
->ifTrue(function ($v) {
return isset($v['version']) && isset($v['json_manifest_path']);
})
->thenInvalid('You cannot use both "version" and "json_manifest_path" at the same time under "assets" packages.')
->end()
->end()
->end()
->end()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ private function registerAssetsConfiguration(array $config, ContainerBuilder $co
if ($config['version_strategy']) {
$defaultVersion = new Reference($config['version_strategy']);
} else {
$defaultVersion = $this->createVersion($container, $config['version'], $config['version_format'], '_default');
$defaultVersion = $this->createVersion($container, $config['version'], $config['version_format'], $config['json_manifest_path'], '_default');
}

$defaultPackage = $this->createPackageDefinition($config['base_path'], $config['base_urls'], $defaultVersion);
Expand All @@ -820,11 +820,14 @@ private function registerAssetsConfiguration(array $config, ContainerBuilder $co
foreach ($config['packages'] as $name => $package) {
if (null !== $package['version_strategy']) {
$version = new Reference($package['version_strategy']);
} elseif (!array_key_exists('version', $package)) {
} elseif (!array_key_exists('version', $package) && null === $package['json_manifest_path']) {
// if neither version nor json_manifest_path are specified, use the default
$version = $defaultVersion;
} else {
// let format fallback to main version_format
$format = $package['version_format'] ?: $config['version_format'];
$version = $this->createVersion($container, $package['version'], $format, $name);
$version = isset($package['version']) ? $package['version'] : null;
$version = $this->createVersion($container, $version, $format, $package['json_manifest_path'], $name);
}

$container->setDefinition('assets._package_'.$name, $this->createPackageDefinition($package['base_path'], $package['base_urls'], $version));
Expand Down Expand Up @@ -856,20 +859,30 @@ private function createPackageDefinition($basePath, array $baseUrls, Reference $
return $package;
}

private function createVersion(ContainerBuilder $container, $version, $format, $name)
private function createVersion(ContainerBuilder $container, $version, $format, $jsonManifestPath, $name)
{
if (null === $version) {
return new Reference('assets.empty_version_strategy');
// Configuration prevents $version and $jsonManifestPath from being set
if (null !== $version) {
$def = new ChildDefinition('assets.static_version_strategy');
$def
->replaceArgument(0, $version)
->replaceArgument(1, $format)
;
$container->setDefinition('assets._version_'.$name, $def);

return new Reference('assets._version_'.$name);
}

$def = new ChildDefinition('assets.static_version_strategy');
$def
->replaceArgument(0, $version)
->replaceArgument(1, $format)
;
$container->setDefinition('assets._version_'.$name, $def);
if (null !== $jsonManifestPath) {
$def = new ChildDefinition('assets.json_manifest_version_strategy');
$def->replaceArgument(0, $jsonManifestPath);
$def->setPublic(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to xml?

$container->setDefinition('assets._version_'.$name, $def);

return new Reference('assets._version_'.$name);
}

return new Reference('assets._version_'.$name);
return new Reference('assets.empty_version_strategy');
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@

<service id="assets.empty_version_strategy" class="Symfony\Component\Asset\VersionStrategy\EmptyVersionStrategy" public="false" />

<service id="assets.json_manifest_version_strategy" class="Symfony\Component\Asset\VersionStrategy\JsonManifestVersionStrategy" abstract="true">
<argument /> <!-- manifest path -->
</service>

<service id="assets.preload_manager" class="Symfony\Component\Asset\Preload\PreloadManager" public="false" />

<service id="asset.preload_listener" class="Symfony\Component\Asset\EventListener\PreloadListener">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@
<xsd:attribute name="version-strategy" type="xsd:string" />
<xsd:attribute name="version" type="xsd:string" />
<xsd:attribute name="version-format" type="xsd:string" />
<xsd:attribute name="json-manifest-path" type="xsd:string" />
</xsd:complexType>

<xsd:complexType name="package">
Expand All @@ -143,6 +144,7 @@
<xsd:attribute name="version-strategy" type="xsd:string" />
<xsd:attribute name="version" type="xsd:string" />
<xsd:attribute name="version-format" type="xsd:string" />
<xsd:attribute name="json-manifest-path" type="xsd:string" />
</xsd:complexType>

<xsd:complexType name="templating">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use PHPUnit\Framework\TestCase;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Configuration;
use Symfony\Bundle\FullStack;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\Config\Definition\Processor;

class ConfigurationTest extends TestCase
Expand Down Expand Up @@ -120,54 +121,67 @@ public function testAssetsCanBeEnabled()
'base_path' => '',
'base_urls' => array(),
'packages' => array(),
'json_manifest_path' => null,
);

$this->assertEquals($defaultConfig, $config['assets']);
}

/**
* @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException
* @expectedExceptionMessage You cannot use both "version_strategy" and "version" at the same time under "assets".
* @dataProvider provideInvalidAssetConfigurationTests
*/
public function testInvalidVersionStrategy()
public function testInvalidAssetsConfiguration(array $assetConfig, $expectedMessage)
{
$this->{method_exists($this, $_ = 'expectException') ? $_ : 'setExpectedException'}(
InvalidConfigurationException::class,
$expectedMessage
);
if (method_exists($this, 'expectExceptionMessage')) {
$this->expectExceptionMessage($expectedMessage);
}

$processor = new Processor();
$configuration = new Configuration(true);
$processor->processConfiguration($configuration, array(
array(
'assets' => array(
'base_urls' => '//example.com',
'version' => 1,
'version_strategy' => 'foo',
array(
'assets' => $assetConfig,
),
),
));
));
}

/**
* @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException
* @expectedExceptionMessage You cannot use both "version_strategy" and "version" at the same time under "assets" packages.
*/
public function testInvalidPackageVersionStrategy()
public function provideInvalidAssetConfigurationTests()
{
$processor = new Processor();
$configuration = new Configuration(true);

$processor->processConfiguration($configuration, array(
array(
'assets' => array(
'base_urls' => '//example.com',
'version' => 1,
'packages' => array(
'foo' => array(
'base_urls' => '//example.com',
'version' => 1,
'version_strategy' => 'foo',
),
),
// helper to turn config into embedded package config
$createPackageConfig = function (array $packageConfig) {
return array(
'base_urls' => '//example.com',
'version' => 1,
'packages' => array(
'foo' => $packageConfig,
),
),
));
);
};

$config = array(
'version' => 1,
'version_strategy' => 'foo',
);
yield array($config, 'You cannot use both "version_strategy" and "version" at the same time under "assets".');
yield array($createPackageConfig($config), 'You cannot use both "version_strategy" and "version" at the same time under "assets" packages.');

$config = array(
'json_manifest_path' => '/foo.json',
'version_strategy' => 'foo',
);
yield array($config, 'You cannot use both "version_strategy" and "json_manifest_path" at the same time under "assets".');
yield array($createPackageConfig($config), 'You cannot use both "version_strategy" and "json_manifest_path" at the same time under "assets" packages.');

$config = array(
'json_manifest_path' => '/foo.json',
'version' => '1',
);
yield array($config, 'You cannot use both "version" and "json_manifest_path" at the same time under "assets".');
yield array($createPackageConfig($config), 'You cannot use both "version" and "json_manifest_path" at the same time under "assets" packages.');
}

protected static function getBundleDefaultConfig()
Expand Down Expand Up @@ -274,6 +288,7 @@ protected static function getBundleDefaultConfig()
'base_path' => '',
'base_urls' => array(),
'packages' => array(),
'json_manifest_path' => null,
),
'cache' => array(
'pools' => array(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
'base_urls' => array('https://bar2.example.com'),
'version_strategy' => 'assets.custom_version_strategy',
),
'json_manifest_strategy' => array(
'json_manifest_path' => '/path/to/manifest.json',
),
),
),
));
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<framework:package name="bar_version_strategy" version-strategy="assets.custom_version_strategy">
<framework:base-url>https://bar_version_strategy.example.com</framework:base-url>
</framework:package>
<framework:package name="json_manifest_strategy" json-manifest-path="/path/to/manifest.json" />
</framework:assets>
</framework:config>
</container>
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ framework:
bar_version_strategy:
base_urls: ["https://bar_version_strategy.example.com"]
version_strategy: assets.custom_version_strategy
json_manifest_strategy:
json_manifest_path: '/path/to/manifest.json'
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public function testAssets()

// packages
$packages = $packages->getArgument(1);
$this->assertCount(5, $packages);
$this->assertCount(6, $packages);

$package = $container->getDefinition((string) $packages['images_path']);
$this->assertPathPackage($container, $package, '/foo', 'SomeVersionScheme', '%%s?version=%%s');
Expand All @@ -390,6 +390,11 @@ public function testAssets()

$package = $container->getDefinition((string) $packages['bar_version_strategy']);
$this->assertEquals('assets.custom_version_strategy', (string) $package->getArgument(1));

$package = $container->getDefinition((string) $packages['json_manifest_strategy']);
$versionStrategy = $container->getDefinition((string) $package->getArgument(1));
$this->assertEquals('assets.json_manifest_version_strategy', $versionStrategy->getParent());
$this->assertEquals('/path/to/manifest.json', $versionStrategy->getArgument(0));
}

public function testAssetsDefaultVersionStrategyAsService()
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Asset/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
CHANGELOG
=========

3.3.0
-----
* JsonManifestVersionStrategy was added as a way to read final,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added Json...

versioned paths from a JSON manifest file.

2.7.0
-----

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Asset\Tests\VersionStrategy;

use PHPUnit\Framework\TestCase;
use Symfony\Component\Asset\VersionStrategy\JsonManifestVersionStrategy;

class JsonManifestVersionStrategyTest extends TestCase
{
public function testGetVersion()
{
$strategy = $this->createStrategy('manifest-valid.json');

$this->assertEquals('main.123abc.js', $strategy->getVersion('main.js'));
}

public function testApplyVersion()
{
$strategy = $this C45C ->createStrategy('manifest-valid.json');

$this->assertEquals('css/styles.555def.css', $strategy->getVersion('css/styles.css'));
}

public function testApplyVersionWhenKeyDoesNotExistInManifest()
{
$strategy = $this->createStrategy('manifest-valid.json');

$this->assertEquals('css/other.css', $strategy->getVersion('css/other.css'));
}

/**
* @expectedException \RuntimeException
*/
public function testMissingManifestFileThrowsException()
{
$strategy = $this->createStrategy('non-existent-file.json');
$strategy->getVersion('main.js');
}

/**
* @expectedException \RuntimeException
* @expectedExceptionMessage Error parsing JSON
*/
public function testManifestFileWithBadJSONThrowsException()
{
$strategy = $this->createStrategy('manifest-invalid.json');
$strategy->getVersion('main.js');
}

private function createStrategy($manifestFilename)
{
return new JsonManifestVersionStrategy(__DIR__.'/../fixtures/'.$manifestFilename);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"main.js": main.123abc.js",
"css/styles.css": "css/styles.555def.css"
}
Loading
0