8000 Implement service-based Resource (cache) validation by mpdude · Pull Request #15738 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Implement service-based Resource (cache) validation #15738

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.

Sign up for GitHub

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 6 commits into from
Closed

Implement service-based Resource (cache) validation #15738

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
535131d
Implement service-based Resource (cache) validation
mpdude Sep 9, 2015
64b9224
Fix deprecation notice in test
mpdude Sep 9, 2015
6d81c69
Bump require-dev dep because TranslatorCacheTest now needs SelfChecki…
mpdude Sep 9, 2015
ab13ef4
Feedback from GH review
mpdude Sep 9, 2015
d1cb1fd
Add UPGRADING notes
mpdude Sep 14, 2015
a845b3c
CS fix / License added
10000 mpdude Sep 14, 2015
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
Next Next commit
Implement service-based Resource (cache) validation 
Currently, any metadata passed to ConfigCache (namely implementations of ResourceInterface) is serialized to disk. When the ConfigCache is validated, the metadata is unserialized and queried through ResourceInterface::isFresh() to determine whether the caceh is fresh. That way, ResourceInterface implementations cannot interact with services, for example a database connection.

This PR introduces the new concept of ResourceCheckers. Services implementing ResourceCheckerInterface can be tagged as "config_cache.resource_checker" with an optional priority.

Clients that wish to use ConfigCache can then obtain an instance from the config_cache_factory service (which implements ConfigCacheFactoryInterface). The factory will take care of injecting resource checkers into the ConfigCache instance so that they can be used for cache validation.

Checking cache metadata is easy for ResourceCheckers:
- First, the ResourceCheckerInterface::supports() implementation is passed the metadata object in question. If the checker cannot handle the type of resource passed, support() should return false.
- Otherwise, the ResourceCheckerInterface::isFresh() method will be called and given the resource as well as the timestamp at which the cache was initialized. If that method returns false, the cache is considered stale. If it returns true, the resource is considered valid and will *not* be passed to any additional checkers.

BC and migration path:

This PR does not (intend to) break BC but it comes with deprecations. The main reason is that ResourceInterface contains an isFresh() method that does not make sense in the general case of resources.

Thus, ResourceInterface::isFresh() is marked as deprecated and shall be removed in Symfony 3.0. Resource implementations that can (or wish to) be validated in that simple manner can implement the SelfCheckingResourceInterface sub-interface that still contains (and will keep) the isFresh() method. The change should be as simple as changing the "extends" list.

Apart from that, ResourceInterface will be kept as the base interface for resource implementations. Note that ResourceInterface is used in several @api interfaces and thus cannot easily be substituted.

For the Symfony 2.x series, a BCResourceInterfaceChecker will be kept that performs validation through ResourceInterface::isFresh() but will trigger a deprecation warning. The remedy is to either implement a custom ResourceChecker with a priority higher than -1000; or to switch to the aforementioned SelfCheckingResourceInterface which is used at a priority of -990 (without deprecation warning).

The ConfigCache and ConfigCacheFactory classes can be used as previously but do not feature checker-based cache validation.

Outlook and closing remarks:

This PR supersedes #7230, #15692 and works at least in parts towards the goal of #7176.

The ResourceCheckerInterface, ...ConfigCache and ...ConfigCacheFactory no longer need to be aware of the "debug" flag. The different validation rules applied previously are now just a matter of ResourceChecker configuration (i. e. "no checkers" in prod). It might be possible to remove the "debug" flag from Symfony's Router and/or Translator classes in the future as well because it was only passed on to the ConfigCache there.
  • Loading branch information
@mpdude
mpdude committed Sep 14, 2015
commit 535131db39669c200d5b9ba2b54e4bef2b5f8ad2
40 changes: 40 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ConfigCachePass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?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\Bundle\FrameworkBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;

/**
* Adds services tagged config_cache.resource_checker to the config_cache_factory service, ordering them by priority.
*
* @author Matthias Pigulla <mp@webfactory.de>
* @author Benjamin Klotz <bk@webfactory.de>
*/
class ConfigCachePass implements CompilerPassInterface
{
public function process(ContainerBuilder $container)
{
$resourceCheckers = array();
foreach ($container->findTaggedServiceIds('config_cache.resource_checker') as $id => $attributes) {
Copy link
Member

Choose a reason for hiding this comment

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

please rename this variable. $attributes is actually an array of tags here, not of attributes

$priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
$resourceCheckers[$priority][] = new Reference($id);
}

// sort by priority and flatten
krsort($resourceCheckers);
$resourceCheckers = call_user_func_array('array_merge', $resourceCheckers);
Copy link
Member

Choose a reason for hiding this comment

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

this will break in case there is no resource checker (array_merge requires at least 1 argument)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (a few lines above)


$container->getDefinition('config_cache_factory')->addMethodCall('setResourceCheckers', array($resourceCheckers));
}
}
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationExtractorPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationDumperPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\SerializerPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ConfigCachePass;
use Symfony\Component\Debug\ErrorHandler;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\PassConfig;
Expand Down Expand Up @@ -92,6 +93,7 @@ public function build(ContainerBuilder $container)
if ($container->getParameter('kernel.debug')) {
$container->addCompilerPass(new ContainerBuilderDebugDumpPass(), PassConfig::TYPE_AFTER_REMOVING);
$container->addCompilerPass(new CompilerDebugDumpPass(), PassConfig::TYPE_AFTER_REMOVING);
$container->addCompilerPass(new ConfigCachePass());
}
}
}
3 changes: 3 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@
</argument>
<argument type="service" id="router.request_context" on-invalid="ignore" />
<argument type="service" id="logger" on-invalid="ignore" />
<call method="setConfigCacheFactory">
<argument type="service" id="config_cache_factory" />
</call>
</service>

<service id="router" alias="router.default" />
Expand Down
17 changes: 17 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
<parameter key="file_locator.class">Symfony\Component\HttpKernel\Config\FileLocator</parameter>
<parameter key="uri_signer.class">Symfony\Component\HttpKernel\UriSigner</parameter>
<parameter key="request_stack.class">Symfony\Component\HttpFoundation\RequestStack</parameter>
<parameter key="config_cache_factory.class">Symfony\Component\Config\ResourceCheckerConfigCacheFactory</parameter>
<parameter key="config_cache_default_resource_checker.class">Symfony\Component\Config\Resource\DefaultResourceChecker</parameter>
<parameter key="config_cache_bc_resource_interface_checker.class">Symfony\Component\Config\Resource\BCResourceInterfaceChecker</parameter>
Copy link
Member

Choose a reason for hiding this comment

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

please don't introduce new class parameters. such practice is deprecated

</parameters>

<services>
Expand Down Expand Up @@ -63,5 +66,19 @@
<service id="uri_signer" class="%uri_signer.class%">
<argument>%kernel.secret%</argument>
</service>

<service id="config_cache_factory" class="%config_cache_factory.class%" />

<service id="config_cache_default_resource_checker" class="%config_cache_default_resource_checker.class%">
Copy link
Member

Choose a reason for hiding this comment

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

should be a private service (all checkers 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.

and I would not use top-level ids for such inner services

<tag name="config_cache.resource_checker" priority="-990" />
</service>

<!--
This service is deprecated and will be removed in 3.0.
-->
<service id="config_cache_bc_resource_interface_checker" class="%config_cache_bc_resource_interface_checker.class%">
<tag name="config_cache.resource_checker" priority="-1000" />
</service>

</services>
</container>
3 changes: 3 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
<argument key="debug">%kernel.debug%</argument>
</argument>
<argument type="collection" /> <!-- translation resources -->
<call method="setConfigCacheFactory">
<argument type="service" id="config_cache_factory" />
</call>
</service>

<service id="translator.logging" class="Symfony\Component\Translation\LoggingTranslator" public="false">
Expand Down
16 changes: 7 additions & 9 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Command/CacheClearCommand/CacheClearCommandTest.php
View file
Open in desktop
10000
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use Symfony\Bundle\FrameworkBundle\Console\Application;
use Symfony\Bundle\FrameworkBundle\Tests\Command\CacheClearCommand\Fixture\TestAppKernel;
use Symfony\Bundle\FrameworkBundle\Tests\TestCase;
use Symfony\Component\Config\ConfigCache;
use Symfony\Component\Config\ConfigCacheFactory;
use Symfony\Component\Config\Resource\ResourceInterface;
use Symfony\Component\Console\Input\ArrayInput;
use Symfony\Component\Console\Output\NullOutput;
Expand Down Expand Up @@ -47,15 +47,13 @@ public function testCacheIsFreshAfterCacheClearedWithWarmup()
$metaFiles = $finder->files()->in($this->kernel->getCacheDir())->name('*.php.meta');
// simply check that cache is warmed up
$this->assertGreaterThanOrEqual(1, count($metaFiles));
$configCacheFactory = new ConfigCacheFactory(true);
$that = $this;

foreach ($metaFiles as $file) {
$configCache = new ConfigCache(substr($file, 0, -5), true);
$this->assertTrue(
$configCache->isFresh(),
sprintf(
'Meta file "%s" is not fresh',
(string) $file
)
);
$configCacheFactory->cache(substr($file, 0, -5), function () use ($that, $file) {
$that->fail(sprintf('Meta file "%s" is not fresh', (string) $file));
});
}

// check that app kernel file present in meta file of container's cache
Expand Down
54 changes: 54 additions & 0 deletions ...Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/ConfigCachePassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?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\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Reference;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ConfigCachePass;

class ConfigCachePassTest extends \PHPUnit_Framework_TestCase
{
public function testThatCheckersAreProcessedInPriorityOrder()
{
$services = array(
'checker_2' => array(0 => array('priority' => 100)),
'checker_1' => array(0 => array('priority' => 200)),
'checker_3' => array(),
);

$definition = $this->getMock('Symfony\Component\DependencyInjection\Definition');
$container = $this->getMock(
'Symfony\Component\DependencyInjection\ContainerBuilder',
array('findTaggedServiceIds', 'getDefinition', 'hasDefinition')
);

$container->expects($this->atLeastOnce())
->method('findTaggedServiceIds')
->will($this->returnValue($services));
$container->expects($this->atLeastOnce())
->method('getDefinition')
->with('config_cache_factory')
->will($this->returnValue($definition));

$definition->expects($this->once())
->method('addMethodCall')
->with('setResourceCheckers', array(
array(
new Reference('checker_1'),
new Reference('checker_2'),
new Reference('checker_3'),
),
));

$pass = new ConfigCachePass();
$pass->process($container);
}
}
4 changes: 2 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,15 +250,15 @@ public function testTranslator()
);

$calls = $container->getDefinition('translator.default')->getMethodCalls();
$this->assertEquals(array('fr'), $calls[0][1][0]);
$this->assertEquals A377 (array('fr'), $calls[1][1][0]);
}

public function testTranslatorMultipleFallbacks()
{
$container = $this->createContainerFromFile('translator_fallbacks');

$calls = $container->getDefinition('translator.default')->getMethodCalls();
$this->assertEquals(array('en', 'fr'), $calls[0][1][0]);
$this->assertEquals(array('en', 'fr'), $calls[1][1][0]);
}

/**
Expand Down
28 changes: 0 additions & 28 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Translation/TranslatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,34 +104,6 @@ public function testTransWithCachingWithInvalidLocale()
$translator->trans('foo');
}

public function testLoadResourcesWithCaching()
{
$loader = new \Symfony\Component\Translation\Loader\YamlFileLoader();
$resourceFiles = array(
'fr' => array(
__DIR__.'/../Fixtures/Resources/translations/messages.fr.yml',
),
);

// prime the cache
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir, 'resource_files' => $resourceFiles), 'yml');
$translator->setLocale('fr');

$this->assertEquals('répertoire', $translator->trans('folder'));

// do it another time as the cache is primed now
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir), 'yml');
$translator->setLocale('fr');

$this->assertEquals('répertoire', $translator->trans('folder'));

// refresh cache when resources is changed in debug mode.
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir, 'debug' => true), 'yml');
$translator->setLocale('fr');

$this->assertEquals('folder', $translator->trans('folder'));
}

public function testLoadResourcesWithoutCaching()
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why this test fails ?

$translator->setConfigCacheFactory(new \Symfony\Component\Config\ResourceCheckerConfigCacheFactory([
    new \Symfony\Component\Config\Resource\SelfCheckingResourceChecker(),
    new \Symfony\Component\Config\Resource\BCResourceInterfaceChecker()
]));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know, but maybe you want to use the default ConfigCacheFactory in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I used ResourceCheckerConfigCacheFactory, the problems occurs when we want to work with the debug mode, is there a way to pass debug mode for ResourceCheckerConfigCacheFactory like the old one ?

{
$loader = new \Symfony\Component\Translation\Loader\YamlFileLoader();
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"php": ">=5.3.9",
"symfony/asset": "~2.7|~3.0.0",
"symfony/dependency-injection": "~2.8",
"symfony/config": "~2.4",
"symfony/config": "~2.8",
"symfony/event-dispatcher": "~2.8|~3.0.0",
"symfony/http-foundation": "~2.4.9|~2.5,>=2.5.4|~3.0.0",
"symfony/http-kernel": "~2.8",
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/Config/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ Before: `InvalidArgumentException` (variable must contain at least two
distinct elements).
After: the code will work as expected and it will restrict the values of the
`variable` option to just `value`.

* deprecated the `ResourceInterface::isFresh()` method. If you implement custom resource types and they
can be validated that way, make them implement the new `SelfCheckingResourceInterface`.

2.7.0
-----
Expand Down
Loading
0