expand configuration definition and adjust tests, closes #151 #155

alcohol · 2017-04-26T08:23:01Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #151
License	MIT

I would like to point out that the official documentation at http://docs.php-http.org/en/latest/plugins/cache.html does not mention the hash_algo configuration option.

alcohol · 2017-04-26T08:23:54Z

I opted to drop all default values for the config section in favor of letting the plugin handle those instead.

Currently I have not yet figured out how to make it not default methods to an empty array though. This is still a work in progress.

dbu

thanks!

dbu · 2017-04-26T08:32:17Z

DependencyInjection/Configuration.php

+                            ->end()
+                            ->enumNode('cache_key_generator')
+                                ->info('A class implementing Http\Client\Common\Plugin\Cache\Generator\CacheKeyGenerator to generate a PSR-6 cache key')
+                                ->values([null, 'Http\Client\Common\Plugin\Cache\Generator\CacheKeyGenerator'])


please use CacheKeyGenerator::class instead (we are php 5.5 or later so its supported)

No problem.

alcohol · 2017-04-26T08:57:52Z

@stof any idea if I can get the config definition builder to somehow not force methods to always present in the final tree? Only option I see at the moment is to replace prototyped array node with a variable node :-\

stof · 2017-04-26T09:58:41Z

DependencyInjection/Configuration.php

-                            ->scalarNode('default_ttl')->defaultValue(0)->end()
+                            ->enumNode('cache_key_generator')
+                                ->info('A class implementing '.CacheKeyGenerator::class.' to generate a PSR-6 cache key')
+                                ->values([null, CacheKeyGenerator::class])


this looks wrong to me. It does not allow passing custom generators anymore

I don't know if it does an instanceof test or literal string comparison. I guess this needs work.

enumNode is just this: an enum. So the value being configured must be one of the provided values. CacheKeyGenerator::class is just a string. There is nothing about instanceof in string comparisons

Dropped the validation in favor of letting the plugin itself handle it directly.

stof · 2017-04-26T10:00:32Z

DependencyInjection/Configuration.php

@@ -368,34 +370,61 @@ private function addSharedPluginNodes(ArrayNodeDefinition $pluginNode, $disableA
                        ->cannotBeEmpty()
                    ->end()
                    ->arrayNode('config')
-                        ->addDefaultsIfNotSet()


if you remove this, you MUST check whether the config key is defined in the DI extension before using it. Currently, the DI extension relies on the fact that the Configuration class ensures it is always defined, but your change means it is not the case anymore.

Can you elaborate what you mean by this? I don't quite follow.

stof · 2017-04-26T10:01:10Z

DependencyInjection/Configuration.php

+                            ->scalarNode('cache_lifetime')
+                                ->info('The minimum time we should store a cache item')
+                            ->end()
+                            ->scalarNode('default_ttl')


integer node looks even better to me for these

It's always been scalar, so I kept it scalar. Not sure why originally integer was not chosen o_O

probably because support for Symfony 2.3 was needed in the past, which did not have the integer node yet.

stof · 2017-04-26T10:02:14Z

DependencyInjection/Configuration.php

+                                ->info('A class implementing '.CacheKeyGenerator::class.' to generate a PSR-6 cache key')
+                                ->values([null, CacheKeyGenerator::class])
+                            ->end()
+                            ->scalarNode('cache_lifetime')


why removing the default value ?

See my first response on this PR.

stof · 2017-04-26T10:05:00Z

Tests/Resources/Fixtures/config/full.xml

                    <respect-response-cache-directives>X-Foo</respect-response-cache-directives>
                    <respect-response-cache-directives>X-Bar</respect-response-cache-directives>
+                    <methods>GET</methods>
+                    <methods>HEAD</methods>


this is wrong. The XML tag should be named method and we need to have the call ->fixXmlConfig('method') on the parent node.

Prototyped nodes work a bit special in XML, due to the way child tags are handled when parsing XML in SimpleXML (if you put a single method with your current code, you will not get an array).

thus, the tag name will be better with a singular

I'll look into this.

stof · 2017-04-26T10:05:58Z

Tests/Resources/Fixtures/config/full.xml

@@ -44,9 +44,11 @@
                <my_service type="service" service="my_auth_service"/>
            </authentication>
            <cache cache-pool="my_cache_pool" stream-factory="my_other_stream_factory">
-                <config default-ttl="42">
+                <config default-ttl="42" cache-lifetime="2592000" hash-algo="sha1" cache-key-generator="null">
                    <respect-response-cache-directives>X-Foo</respect-response-cache-directives>
                    <respect-response-cache-directives>X-Bar</respect-response-cache-directives>


this prototyped node is also broken in XML btw

stof · 2017-04-26T10:07:19Z

@alcohol prototyped array nodes always have a default value (the empty array, unless you override it).
What you could do is treating an empty array in a special way when using the config in the DI extension.

alcohol · 2017-04-26T10:40:02Z

I resorted to just providing the default value instead. I think that is the easiest approach without having to implement too many work-arounds or do all sorts of additional checks.

dbu

thanks for this!

the integerNode was probably caused by me not being aware of that and having seen scalarNode from previous work - i think we did not actually have this work with symfony 2.3 at any point.

dbu · 2017-04-26T11:30:38Z

DependencyInjection/Configuration.php

+            ->end()
+            ->children()
+                ->scalarNode('cache_key_generator')
+                    ->info('A class implementing '.CacheKeyGenerator::class.' to generate a PSR-6 cache key')


actually the cache plugin phpdoc is confusing on this one. if this option is set, it must contain an instance of CacheKeyGenerator. in symfony config that means it would be a service id.

php-http/cache-plugin#41

So...

This must be a service id to a service implementing Http\Client\Common\Plugin\Cache\Generator\CacheKeyGenerator

?

And then it means that the DI extension has to process the config to turn the id into a Reference object.

Currently, the feature is just broken

dbu · 2017-04-26T12:02:46Z

exactly. if it is set at all - if its empty, the cache plugin creates an object on its own.

alcohol · 2017-04-28T06:56:44Z

Anything that still needs changing/modification?

alcohol · 2017-04-28T06:57:24Z

DependencyInjection/Configuration.php

+
+        $config = $builder->root('config');
+        $config
+            ->fixXmlConfig('method')


@stof Should this be considered a BC btw?

Well, as there is no XSD file, the existing XML config file will continue to work, for people lucky enough to have a working config before (it was broken when having a single method). And it will allow everyone to have a working config when using XML, for all cases.

Btw, it cannot be considered a BC. The sentence should be considered a BC break. The important work in this sentence is break 😄

Nyholm

Thank you

Nyholm self-requested a review April 26, 2017 08:23

dbu reviewed Apr 26, 2017

View reviewed changes

stof suggested changes Apr 26, 2017

View reviewed changes

dbu requested changes Apr 26, 2017

View reviewed changes

dbu mentioned this pull request Apr 26, 2017

clarify cache_key_generator option php-http/cache-plugin#41

Merged

expand configuration definition and adjust tests

937ff0b

alcohol commented Apr 28, 2017

View reviewed changes

dbu approved these changes May 2, 2017

View reviewed changes

Nyholm approved these changes May 2, 2017

View reviewed changes

Nyholm merged commit 79070d6 into php-http:master May 2, 2017

alcohol deleted the expand-cache-plugin-configuration branch May 2, 2017 07:09

expand configuration definition and adjust tests, closes #151 #155

expand configuration definition and adjust tests, closes #151 #155

Uh oh!

Conversation

alcohol commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alcohol commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcohol commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Apr 26, 2017

Uh oh!

alcohol commented Apr 26, 2017

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Apr 26, 2017 via email

Uh oh!

alcohol commented Apr 28, 2017

Uh oh!

alcohol Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alcohol commented Apr 26, 2017 •

edited

Loading

alcohol commented Apr 26, 2017 •

edited

Loading

alcohol Apr 28, 2017 •

edited

Loading