Skip to content

Switch to the new Symfony cache infrastructure #577

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

Merged
merged 1 commit into from
Jun 15, 2016
Merged

Switch to the new Symfony cache infrastructure #577

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
32 changes: 0 additions & 32 deletions src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ public function load(array $configs, ContainerBuilder $container)
$this->enableJsonLd($loader);
$this->registerAnnotationLoaders($container);
$this->registerFileLoaders($container);
$this->setUpMetadataCaching($container, $config);

if (!interface_exists('phpDocumentor\Reflection\DocBlockFactoryInterface')) {
$container->removeDefinition('api_platform.metadata.resource.metadata_factory.php_doc');
Expand Down Expand Up @@ -175,35 +174,4 @@ private function registerFileLoaders(ContainerBuilder $container)
$container->getDefinition('api_platform.metadata.resource.name_collection_factory.xml')->replaceArgument(0, $xmlResources);
$container->getDefinition('api_platform.metadata.resource.metadata_factory.xml')->replaceArgument(0, $xmlResources);
}

/**
* Sets up metadata caching.
*
* @param ContainerBuilder $container
* @param array $config
*/
private function setUpMetadataCaching(ContainerBuilder $container, array $config)
{
$container->setAlias('api_platform.metadata.resource.cache', $config['metadata']['resource']['cache']);
$container->setAlias('api_platform.metadata.property.cache', $config['metadata']['property']['cache']);

if (!class_exists('Symfony\Component\Cache\Adapter\ArrayAdapter')) {
$container->removeDefinition('api_platform.metadata.resource.cache.array');
$container->removeDefinition('api_platform.metadata.resource.cache.apcu');
$container->removeDefinition('api_platform.metadata.property.cache.array');
$container->removeDefinition('api_platform.metadata.property.cache.apcu');
}

if (!$container->has($config['metadata']['resource']['cache'])) {
$container->removeAlias('api_platform.metadata.resource.cache');
$container->removeDefinition('api_platform.metadata.resource.name_collection_factory.cached');
$container->removeDefinition('api_platform.metadata.resource.metadata_factory.cached');
}

if (!$container->has($config['metadata']['property']['cache'])) {
$container->removeAlias('api_platform.metadata.property.cache');
$container->removeDefinition('api_platform.metadata.property.name_collection_factory.cached');
$container->removeDefinition('api_platform.metadata.property.metadata_factory.cached');
}
}
}
17 changes: 0 additions & 17 deletions src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,23 +87,6 @@ public function getConfigTreeBuilder()
->end()
->end()
->end()
->arrayNode('metadata')
->addDefaultsIfNotSet()
->children()
->arrayNode('resource')
->addDefaultsIfNotSet()
->children()
->scalarNode('cache')->defaultValue('api_platform.metadata.resource.cache.array')->cannotBeEmpty()->info('Cache service for resource metadata.')->end()
->end()
->end()
->arrayNode('property')
->addDefaultsIfNotSet()
->children()
->scalarNode('cache')->defaultValue('api_platform.metadata.property.cache.array')->cannotBeEmpty()->info('Cache service for property metadata.')->end()
->end()
->end()
->end()
->end()
->end();

return $treeBuilder;
Expand Down
20 changes: 4 additions & 16 deletions src/Bridge/Symfony/Bundle/Resources/config/metadata.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,24 +116,12 @@

<!-- Cache -->

<service id="api_platform.metadata.resource.cache.array" class="Symfony\Component\Cache\Adapter\ArrayAdapter" public="false">
<argument>0</argument>
<argument>false</argument>
<service id="api_platform.metadata.resource.cache" parent="cache.system" public="false">
Copy link
Contributor

Choose a reason for hiding this comment

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

Pertaining to symfony/symfony#18667, should our cache be considered a "system" or "app" cache? What are the semantics?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can leverage the "system" cache, but if my understanding of the code is correct, these will be used in the dev environment as well... (Still, we should be safe because of the nonce invalidation.)

Copy link
Member Author

Choose a reason for hiding this comment

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

system is the right choice here (system is for local cache, usually for metadata, app is for distributed cache, usually for data).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the semantics should be that "app" refers to the user project (the app that the user builds).

Copy link
Member Author

@dunglas dunglas Jun 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@teohhanhui you can discuss that on the Symfony issue tracker but I doubt it is worth it, it would be a BC break to change that and it's not acceptable as 3.1 has been released.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has nothing to do with Symfony. As far as Symfony is concerned, everything Symfony goes under the "system" cache pool.

In any case, I'm just trying to clarify the semantics, and my conclusion seems to be consistent with yours (at least in this case).

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact this is a nothing to do "directly" with Symfony:

  • system will store the cache on the server (using APC or a file)
  • app will store the cache on a cache server (like Redis or Memcache)

system is better suited for cache that do not need to be shared between servers (like metadata cache).

<tag name="cache.pool" />
</service>

<service id="api_platform.metadata.resource.cache.apcu" class="Symfony\Component\Cache\Adapter\ApcuAdapter" public="false">
<argument>api_platform_metadata_resource</argument>
<argument>0</argument>
</service>

<service id="api_platform.metadata.property.cache.array" class="Symfony\Component\Cache\Adapter\ArrayAdapter" public="false">
<argument>0</argument>
<argument>false</argument>
</service>

<service id="api_platform.metadata.property.cache.apcu" class="Symfony\Component\Cache\Adapter\ApcuAdapter" public="false">
<argument>api_platform_metadata_property</argument>
<argument>0</argument>
<service id="api_platform.metadata.property.cache" parent="cache.system" public="false">
<tag name="cache.pool" />
</service>

</services>
Expand Down
36 changes: 6 additions & 30 deletions tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace ApiPlatform\Core\Tests\Symfony\Bridge\Bundle\DependencyInjection;

use ApiPlatform\Core\Bridge\Symfony\Bundle\DependencyInjection\ApiPlatformExtension;
use Doctrine\Bundle\DoctrineBundle\DoctrineBundle;
use Prophecy\Argument;
use Symfony\Component\Config\Resource\ResourceInterface;
use Symfony\Component\DependencyInjection\Alias;
Expand All @@ -33,9 +34,10 @@ class ApiPlatformExtensionTest extends \PHPUnit_Framework_TestCase
'description' => 'description',
],
];

private $extension;

public function setUp()
protected function setUp()
{
$this->extension = new ApiPlatformExtension();
}
Expand Down Expand Up @@ -152,25 +154,6 @@ public function testEnableNelmioApiDoc()
$this->extension->load(array_merge_recursive(self::DEFAULT_CONFIG, ['api_platform' => ['enable_nelmio_api_doc' => true]]), $containerBuilder);
}

public function testSetApcuMetadataCache()
{
$containerBuilderProphecy = $this->getContainerBuilderProphecy();
$containerBuilderProphecy->setAlias('api_platform.metadata.resource.cache', 'api_platform.metadata.resource.cache.apcu')->shouldBeCalled();
$containerBuilderProphecy->setAlias('api_platform.metadata.resource.cache', 'api_platform.metadata.resource.cache.array')->shouldNotBeCalled();
$containerBuilderProphecy->setAlias('api_platform.metadata.property.cache', 'api_platform.metadata.property.cache.apcu')->shouldBeCalled();
$containerBuilderProphecy->setAlias('api_platform.metadata.property.cache', 'api_platform.metadata.property.cache.array')->shouldNotBeCalled();
$containerBuilderProphecy->has('api_platform.metadata.resource.cache.apcu')->willReturn(true)->shouldBeCalled();
$containerBuilderProphecy->has('api_platform.metadata.resource.cache.array')->shouldNotBeCalled();
$containerBuilderProphecy->has('api_platform.metadata.property.cache.apcu')->willReturn(true)->shouldBeCalled();
$containerBuilderProphecy->has('api_platform.metadata.property.cache.array')->shouldNotBeCalled();
$containerBuilder = $containerBuilderProphecy->reveal();

$this->extension->load(array_merge_recursive(self::DEFAULT_CONFIG, ['api_platform' => ['metadata' => [
'resource' => ['cache' => 'api_platform.metadata.resource.cache.apcu'],
'property' => ['cache' => 'api_platform.metadata.property.cache.apcu'],
]]]), $containerBuilder);
}

private function getContainerBuilderProphecy()
{
$definitionArgument = Argument::that(function ($argument) {
Expand All @@ -179,7 +162,7 @@ private function getContainerBuilderProphecy()

$containerBuilderProphecy = $this->prophesize(ContainerBuilder::class);
$containerBuilderProphecy->getParameter('kernel.bundles')->willReturn([
'DoctrineBundle' => 'Doctrine\Bundle\DoctrineBundle\DoctrineBundle',
'DoctrineBundle' => DoctrineBundle::class,
Copy link
Contributor

Choose a reason for hiding this comment

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

There are other places where this is not changed. Perhaps we should make this consistent... (and for NelmioApiDocBundle as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 should be changed everywhere to the class constant.

])->shouldBeCalled();

$parameters = [
Expand Down Expand Up @@ -259,17 +242,15 @@ private function getContainerBuilderProphecy()
'api_platform.metadata.resource.metadata_factory.short_name',
'api_platform.metadata.resource.metadata_factory.operation',
'api_platform.metadata.resource.metadata_factory.cached',
'api_platform.metadata.resource.cache.array',
'api_platform.metadata.resource.cache.apcu',
'api_platform.metadata.resource.cache',
'api_platform.metadata.property.name_collection_factory.property_info',
'api_platform.metadata.property.name_collection_factory.cached',
'api_platform.metadata.property.metadata_factory.annotation',
'api_platform.metadata.property.metadata_factory.property_info',
'api_platform.metadata.property.metadata_factory.serializer',
'api_platform.metadata.property.metadata_factory.validator',
'api_platform.metadata.property.metadata_factory.cached',
'api_platform.metadata.property.cache.array',
'api_platform.metadata.property.cache.apcu',
'api_platform.metadata.property.cache',
'api_platform.negotiator',
'api_platform.route_loader',
'api_platform.router',
Expand Down Expand Up @@ -328,17 +309,12 @@ private function getContainerBuilderProphecy()
$aliases = [
'api_platform.routing.resource_path_generator' => 'api_platform.routing.resource_path_generator.underscore',
'api_platform.metadata.resource.name_collection_factory' => 'api_platform.metadata.resource.name_collection_factory.annotation',
'api_platform.metadata.resource.cache' => 'api_platform.metadata.resource.cache.array',
'api_platform.metadata.property.cache' => 'api_platform.metadata.property.cache.array',
];

foreach ($aliases as $alias => $service) {
$containerBuilderProphecy->setAlias($alias, $service)->shouldBeCalled();
}

$containerBuilderProphecy->has('api_platform.metadata.resource.cache.array')->willReturn(true)->shouldBeCalled();
$containerBuilderProphecy->has('api_platform.metadata.property.cache.array')->willReturn(true)->shouldBeCalled();

return $containerBuilderProphecy;
}
}
8 changes: 0 additions & 8 deletions tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,6 @@ public function testDefaultConfig()
'items_per_page_parameter_name' => 'itemsPerPage',
],
],
'metadata' => [
'resource' => [
'cache' => 'api_platform.metadata.resource.cache.array',
],
'property' => [
'cache' => 'api_platform.metadata.property.cache.array',
],
],
], $config);
}
}