Skip to content

Commit 194e8a5

Browse files
author
Till Hildebrandt
committed
Pull-Request Feedback for "Wrapped php-http/cache-plugin Cache Listeners"
 - switched from list of classes to list of services - removed class implementation validation in configuration
1 parent b1ccb54 commit 194e8a5

File tree

5 files changed

+22
-71
lines changed

5 files changed

+22
-71
lines changed

‎src/DependencyInjection/Configuration.php‎

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
use Symfony\Component\Config\Definition\ConfigurationInterface;
2222
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
2323
use Symfony\Component\Config\Definition\Exception\InvalidTypeException;
24+
use Symfony\Component\DependencyInjection\Definition;
25+
use Symfony\Component\DependencyInjection\Reference;
2426

2527
/**
2628
* This class contains the configuration information for the bundle.
@@ -742,24 +744,9 @@ private function createCachePluginNode()
742744
->end()
743745
->end()
744746
->arrayNode('cache_listeners')
745-
->info('An array of classes to act on the response based on the results of the cache check. Must implement '.CacheListener::class.'. Defaults to an empty array.')
746-
->beforeNormalization()->castToArray()->ifEmpty()->thenUnset()->end()
747-
->defaultValue([])
747+
->info('A list of service ids to act on the response based on the results of the cache check. Must implement '.CacheListener::class.'. Defaults to an empty array.')
748+
->beforeNormalization()->ifNull()->thenEmptyArray()->castToArray()->ifEmpty()->thenUnset()->end()
748749
->prototype('scalar')
749-
->validate()
750-
->ifTrue(function ($v) {
751-
$vs = is_array($v) ? $v : (is_null($v) ? [] : [$v]);
752-
753-
return empty($vs) || array_reduce($vs, function ($r, $e) {
754-
return empty($e) || !class_exists($e) || !(new ReflectionClass($e))->implementsInterface(CacheListener::class);
755-
}, false);
756-
})
757-
->thenInvalid('A given listener class does not implement '.CacheListener::class)
758-
->end()
759-
->end()
760-
->validate()
761-
->ifEmpty()
762-
->thenUnset()
763750
->end()
764751
->end()
765752
->scalarNode('respect_cache_headers')

‎src/DependencyInjection/HttplugExtension.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ private function configurePluginByName($name, Definition $definition, array $con
211211
if (!empty($options['cache_listeners'])) {
212212
foreach ($options['cache_listeners'] as $i => $listener) {
213213
if (!empty($listener)) {
214-
$options['cache_listeners'][$i] = new Definition($listener);
214+
$options['cache_listeners'][$i] = new Reference($listener);
215215
}
216216
}
217217
}

‎tests/Resources/Fixtures/config/invalid_cache_listener_config.yml‎

Lines changed: 0 additions & 6 deletions
This file was deleted.

‎tests/Unit/DependencyInjection/ConfigurationTest.php‎

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
namespace Http\HttplugBundle\Tests\Unit\DependencyInjection;
44

5-
use Http\Client\Common\Plugin\Cache\Listener\CacheListener;
65
use Http\HttplugBundle\DependencyInjection\Configuration;
76
use Http\HttplugBundle\DependencyInjection\HttplugExtension;
87
use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractExtensionConfigurationTestCase;
@@ -97,7 +96,7 @@ protected function getConfiguration(): ConfigurationInterface
9796
public function testEmptyConfiguration(): void
9897
{
9998
$formats = array_map(function ($path) {
100-
return __DIR__.'/../../Resources/Fixtures/'.$path;
99+
return __DIR__ . '/../../Resources/Fixtures/' . $path;
101100
}, [
102101
'config/empty.yml',
103102
'config/empty.xml',
@@ -278,7 +277,7 @@ public function testSupportsAllConfigFormats(): void
278277
];
279278

280279
$formats = array_map(function ($path) {
281-
return __DIR__.'/../../Resources/Fixtures/'.$path;
280+
return __DIR__ . '/../../Resources/Fixtures/' . $path;
282281
}, [
283282
'config/full.yml',
284283
'config/full.xml',
@@ -292,7 +291,7 @@ public function testSupportsAllConfigFormats(): void
292291

293292
public function testMissingClass(): void
294293
{
295-
$file = __DIR__.'/../../Resources/Fixtures/config/invalid_class.yml';
294+
$file = __DIR__ . '/../../Resources/Fixtures/config/invalid_class.yml';
296295

297296
$this->expectException(InvalidConfigurationException::class);
298297
$this->expectExceptionMessage('Nonexisting\Class');
@@ -301,7 +300,7 @@ public function testMissingClass(): void
301300

302301
public function testInvalidPlugin(): void
303302
{
304-
$file = __DIR__.'/../../Resources/Fixtures/config/invalid_plugin.yml';
303+
$file = __DIR__ . '/../../Resources/Fixtures/config/invalid_plugin.yml';
305304

306305
$this->expectException(InvalidConfigurationException::class);
307306
$this->expectExceptionMessage('Unrecognized option "foobar" under "httplug.clients.acme.plugins.0"');
@@ -310,7 +309,7 @@ public function testInvalidPlugin(): void
310309

311310
public function testInvalidAuthentication(): void
312311
{
313-
$file = __DIR__.'/../../Resources/Fixtures/config/invalid_auth.yml';
312+
$file = __DIR__ . '/../../Resources/Fixtures/config/invalid_auth.yml';
314313

315314
$this->expectException(InvalidConfigurationException::class);
316315
$this->expectExceptionMessage('password, service, username');
@@ -322,7 +321,7 @@ public function testInvalidAuthentication(): void
322321
*/
323322
public function testInvalidCacheConfig(): void
324323
{
325-
$file = __DIR__.'/../../Resources/Fixtures/config/invalid_cache_config.yml';
324+
$file = __DIR__ . '/../../Resources/Fixtures/config/invalid_cache_config.yml';
326325

327326
$this->expectException(InvalidConfigurationException::class);
328327
$this->expectExceptionMessage('Invalid configuration for path "httplug.plugins.cache.config": You can\'t provide config option "respect_cache_headers" and "respect_response_cache_directives" simultaniously. Use "respect_response_cache_directives" instead.');
@@ -335,7 +334,7 @@ public function testInvalidCacheConfig(): void
335334
public function testBackwardCompatibility(): void
336335
{
337336
$formats = array_map(function ($path) {
338-
return __DIR__.'/../../Resources/Fixtures/'.$path;
337+
return __DIR__ . '/../../Resources/Fixtures/' . $path;
339338
}, [
340339
'config/bc/toolbar.yml',
341340
'config/bc/toolbar_auto.yml',
@@ -351,7 +350,7 @@ public function testBackwardCompatibility(): void
351350
*/
352351
public function testCacheConfigDeprecationCompatibility(): void
353352
{
354-
$file = __DIR__.'/../../Resources/Fixtures/config/bc/cache_config.yml';
353+
$file = __DIR__ . '/../../Resources/Fixtures/config/bc/cache_config.yml';
355354
$config = $this->emptyConfig;
356355
$config['plugins']['cache'] = array_merge($config['plugins']['cache'], [
357356
'enabled' => true,
@@ -370,7 +369,7 @@ public function testCacheConfigDeprecationCompatibility(): void
370369
*/
371370
public function testCacheConfigDeprecationCompatibilityIssue166(): void
372371
{
373-
$file = __DIR__.'/../../Resources/Fixtures/config/bc/issue-166.yml';
372+
$file = __DIR__ . '/../../Resources/Fixtures/config/bc/issue-166.yml';
374373
$config = $this->emptyConfig;
375374
$config['plugins']['cache'] = array_merge($config['plugins']['cache'], [
376375
'enabled' => true,
@@ -386,7 +385,7 @@ public function testCacheConfigDeprecationCompatibilityIssue166(): void
386385

387386
public function testProfilingToolbarCollision(): void
388387
{
389-
$file = __DIR__.'/../../Resources/Fixtures/config/bc/profiling_toolbar.yml';
388+
$file = __DIR__ . '/../../Resources/Fixtures/config/bc/profiling_toolbar.yml';
390389

391390
$this->expectException(InvalidConfigurationException::class);
392391
$this->expectExceptionMessage('Can\'t configure both "toolbar" and "profiling" section. The "toolbar" config is deprecated as of version 1.3.0, please only use "profiling".');
@@ -395,7 +394,7 @@ public function testProfilingToolbarCollision(): void
395394

396395
public function testClientCacheConfigMustHavePool(): void
397396
{
398-
$file = __DIR__.'/../../Resources/Fixtures/config/client_cache_config_with_no_pool.yml';
397+
$file = __DIR__ . '/../../Resources/Fixtures/config/client_cache_config_with_no_pool.yml';
399398

400399
$this->expectException(InvalidConfigurationException::class);
401400
$this->expectExceptionMessage('The child node "cache_pool" at path "httplug.clients.test.plugins.0.cache" must be configured.');
@@ -404,7 +403,7 @@ public function testClientCacheConfigMustHavePool(): void
404403

405404
public function testCacheConfigMustHavePool(): void
406405
{
407-
$file = __DIR__.'/../../Resources/Fixtures/config/cache_config_with_no_pool.yml';
406+
$file = __DIR__ . '/../../Resources/Fixtures/config/cache_config_with_no_pool.yml';
408407

409408
$this->expectException(InvalidConfigurationException::class);
410409
$this->expectExceptionMessage('The child node "cache_pool" at path "httplug.plugins.cache" must be configured.');
@@ -413,28 +412,18 @@ public function testCacheConfigMustHavePool(): void
413412

414413
public function testLimitlessCapturedBodyLength(): void
415414
{
416-
$file = __DIR__.'/../../Resources/Fixtures/config/limitless_captured_body_length.yml';
415+
$file = __DIR__ . '/../../Resources/Fixtures/config/limitless_captured_body_length.yml';
417416
$config = $this->emptyConfig;
418417
$config['profiling']['captured_body_length'] = null;
419418
$this->assertProcessedConfigurationEquals($config, [$file]);
420419
}
421420

422421
public function testInvalidCapturedBodyLengthString(): void
423422
{
424-
$file = __DIR__.'/../../Resources/Fixtures/config/invalid_captured_body_length.yml';
423+
$file = __DIR__ . '/../../Resources/Fixtures/config/invalid_captured_body_length.yml';
425424

426425
$this->expectException(InvalidConfigurationException::class);
427426
$this->expectExceptionMessage('The child node "captured_body_length" at path "httplug.profiling" must be an integer or null');
428427
$this->assertProcessedConfigurationEquals([], [$file]);
429428
}
430-
431-
public function testInvalidCacheConfigCacheListeners(): void
432-
{
433-
$file = __DIR__.'/../../Resources/Fixtures/config/invalid_cache_listener_config.yml';
434-
435-
$this->expectException(InvalidConfigurationException::class);
436-
$this->expectExceptionMessage('A given listener class does not implement '.CacheListener::class);
437-
438-
$this->assertProcessedConfigurationEquals($this->emptyConfig, [$file]);
439-
}
440429
}

‎tests/Unit/DependencyInjection/HttplugExtensionTest.php‎

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
use Http\HttplugBundle\Collector\PluginClientFactoryListener;
88
use Http\HttplugBundle\DependencyInjection\HttplugExtension;
99
use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractExtensionTestCase;
10-
use Symfony\Component\DependencyInjection\Definition;
1110
use Symfony\Component\DependencyInjection\Reference;
1211
use Symfony\Component\HttpKernel\Kernel;
1312
use Http\Adapter\Guzzle6\Client;
@@ -270,7 +269,7 @@ public function testCachePluginConfigCacheListenersDefinition(): void
270269
'cache_pool' => 'my_cache_pool',
271270
'config' => [
272271
'cache_listeners' => [
273-
'\Http\Client\Common\Plugin\Cache\Listener\AddHeaderCacheListener',
272+
'httplug.plugin.cache.listeners.add_header',
274273
],
275274
],
276275
],
@@ -281,26 +280,8 @@ public function testCachePluginConfigCacheListenersDefinition(): void
281280

282281
$config = $cachePlugin->getArgument(2);
283282
$this->assertArrayHasKey('cache_listeners', $config);
284-
$this->assertContainsOnlyInstancesOf(Definition::class, $config['cache_listeners']);
285-
}
286-
287-
public function testCachePluginInvalidConfigCacheListenersDefinition(): void
288-
{
289-
$this->load([
290-
'plugins' => [
291-
'cache' => [
292-
'cache_pool' => 'my_cache_pool',
293-
'config' => [
294-
'cache_listeners' => [],
295-
],
296-
],
297-
],
298-
]);
299-
300-
$cachePlugin = $this->container->findDefinition('httplug.plugin.cache');
301-
302-
$config = $cachePlugin->getArgument(2);
303-
$this->assertArrayNotHasKey('cache_listeners', $config);
283+
$this->assertContainsOnlyInstancesOf(Reference::class, $config['cache_listeners']);
284+
$this->assertSame('httplug.plugin.cache.listeners.add_header', (string) $config['cache_listeners'][0]);
304285
}
305286

306287
public function testContentTypePluginAllowedOptions(): void

0 commit comments

Comments
 (0)