Skip to content
This repository was archived by the owner on Jan 1, 2023. It is now read-only.

Added the support of the CacheBustingWorker #119

Closed
wants to merge 1 commit into from
Closed

Added the support of the CacheBustingWorker #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions DependencyInjection/AsseticExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ public function load(array $configs, ContainerBuilder $container)
}

$container->setParameter('assetic.bundles', $config['bundles']);

$container->setParameter('assetic.worker.cache_busting.strategy', $config['workers']['cache_busting']['strategy']);
if ($config['workers']['cache_busting']['enabled']) {
$container->getDefinition('assetic.worker.cache_busting')->addTag('assetic.factory_worker');
}
}

/**
Expand Down
26 changes: 26 additions & 0 deletions DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Bundle\AsseticBundle\DependencyInjection;

use Assetic\Factory\Worker\CacheBustingWorker;
use Symfony\Component\Process\ExecutableFinder;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;
Expand Down Expand Up @@ -172,6 +173,31 @@ public function getConfigTreeBuilder()
->end()
->end()

// workers
->children()
->arrayNode('workers')
->addDefaultsIfNotSet()
->children()
->arrayNode('cache_busting')
->addDefaultsIfNotSet()
->children()
->booleanNode('enabled')->defaultFalse()->end()
->enumNode('strategy')
->values(array(CacheBustingWorker::STRATEGY_CONTENT, CacheBustingWorker::STRATEGY_MODIFICATION))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use strings rather than constants here and translate them downstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

you can use strings. See the ->beforeNormalization() on this node.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we change the value of the constants to be integers in the future, config:dump-reference would look very strange…

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, config:dump-reference should probably be improved to be able take the normalization into account. But converting the string to the constant is exactly the job of the normalization

Copy link
Contributor

Choose a reason for hiding this comment

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

All I'm suggesting is that we change this line to…

->values(array('content', 'modification'))

…and the default value to 'content'.

Copy link
Member Author

Choose a reason for hiding this comment

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

and then, you are moving the configuration normalization work to the DI extension instead of the configuration tree where it belongs

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, normalization is okay here.

->defaultValue(CacheBustingWorker::STRATEGY_CONTENT)
->beforeNormalization()
->ifString()
->then(function($v) {
return constant('Assetic\Factory\Worker\CacheBustingWorker::STRATEGY_'.strtoupper($v));
})
->end()
->end()
->end()
->end()
->end()
->end()
->end()

// twig
->children()
->arrayNode('twig')
Expand Down
9 changes: 7 additions & 2 deletions Resources/config/assetic.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<parameter key="assetic.directory_resource.class">Symfony\Bundle\AsseticBundle\Factory\Resource\DirectoryResource</parameter>
<parameter key="assetic.filter_manager.class">Symfony\Bundle\AsseticBundle\FilterManager</parameter>
<parameter key="assetic.worker.ensure_filter.class">Assetic\Factory\Worker\EnsureFilterWorker</parameter>
<parameter key="assetic.worker.cache_busting.class">Assetic\Factory\Worker\CacheBustingWorker</parameter>
<parameter key="assetic.value_supplier.class">Symfony\Bundle\AsseticBundle\DefaultValueSupplier</parameter>

<parameter key="assetic.node.paths" type="collection"></parameter>
Expand Down Expand Up @@ -64,12 +65,16 @@
<argument /> <!-- filter -->
</service>

<service id="assetic.worker.cache_busting" class="%assetic.worker.cache_busting.class%" public="false">
<argument>%assetic.worker.cache_busting.strategy%</argument>
</service>

<service id="assetic.parameter_bag" class="Symfony\Component\DependencyInjection\ParameterBag\ParameterBag" factory-service="service_container" factory-method="getParameterBag" public="false"/>

<service id="assetic.value_supplier.default" class="%assetic.value_supplier.class%" public="false">
<argument type="service" id="service_container" />
</service>

<service id="assetic.value_supplier" alias="assetic.value_supplier.default" public="false" />
</services>
</container>
22 changes: 22 additions & 0 deletions Tests/DependencyInjection/AsseticExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,28 @@ public function getUseControllerKeys()
);
}

/**
* @dataProvider getCacheBustingWorkerKeys
*/
public function testCacheBustingWorker($enabled)
{
$extension = new AsseticExtension();
$extension->load(array(array('workers' => array('cache_busting' => array('enabled' => $enabled)))), $this->container);

$def = $this->container->getDefinition('assetic.worker.cache_busting');
$this->assertSame($enabled, $def->hasTag('assetic.factory_worker'));

$this->assertSaneContainer($this->getDumpedContainer());
}

public function getCacheBustingWorkerKeys()
Copy link

Choose a reason for hiding this comment

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

IMO thats worth two different tests

  • They test two different cases, not just different values for the same case, or similar cases.
  • You'll probably never add more values, to cover other cases, because there only two boolean values.

{
return array(
array(true),
array(false),
);
}

/**
* @dataProvider getClosureJarAndExpected
*/
Expand Down