replace deprecated directive with new one #145

alcohol · 2017-04-06T13:02:32Z

No description provided.

Nyholm

Thank you! Just a small issue

Nyholm · 2017-04-06T13:10:35Z

DependencyInjection/Configuration.php

@@ -370,7 +370,7 @@ private function addSharedPluginNodes(ArrayNodeDefinition $pluginNode, $disableA
                        ->addDefaultsIfNotSet()
                        ->children()
                            ->scalarNode('default_ttl')->defaultNull()->end()
-                            ->scalarNode('respect_cache_headers')->defaultTrue()->end()
+                            ->arrayNode('respect_response_cache_directives')->defaultNull()->end()


We cannot remove respect_cache_headers because that would be a BC break.
We should have both in the configuration.

But having both configured triggers a \InvalidArgumentException: https://github.com/php-http/cache-plugin/blob/master/src/CachePlugin.php#L69-L74

What do you propose? Configure both with defaultNull?

We should use scalarNode('respect_cache_headers')->defaultNull()->end(). That would be the same as "not using it".

Pushed an amendment. Looks better now?

alcohol · 2017-04-10T11:00:21Z

Looking at the diff now, I feel like the merge didn't go entirely ok. Either that or I need to make some amendments.

Nyholm · 2017-04-10T11:26:01Z

Did I do something wrong? Aren't the diff as you intended now?

alcohol · 2017-04-10T18:44:18Z

Tests/Unit/DependencyInjection/ConfigurationTest.php looks wrong. Double entries in the array.

Nyholm · 2017-04-14T08:47:28Z

Yes, and it should be 0 and not null, Can you update that? I do not think I can without stealing cred from you.

stof · 2017-04-19T07:44:58Z

DependencyInjection/Configuration.php

+                            ->variableNode('respect_response_cache_directives')
+                                ->validate()
+                                ->always(function ($v) {
+                                    if (is_null($v) || is_array($v)) {


if it must always be an array, use prototyped array node rather than a variable node. It will allow better handling of the data.

Rule of thumb: if you start using variableNode, rethink what you are doing. Most of the time, you should not need it.

Except it can also be null.

stof · 2017-04-19T08:07:57Z

you need to convert respect_cache_headers to the new way and trigger a deprecation warning.
The existing config must keep working for projects using it, but they should be warned that they should migrate their config to the new way.

alcohol · 2017-04-20T13:04:16Z

I'll have more time next week at the earliest unfortunately.

alcohol · 2017-04-25T07:35:03Z

@stof what approach do you suggest? Converting it is not entirely straightforward since the formats are different (null | bool vs null | array).

stof · 2017-04-25T08:27:31Z

@alcohol what are the new configurations corresponding to true and false ?

The other option would be to rely on a BC layer at the library level (which should be here anyway) and only add a validation rule forbidding to configure both the old and the new way at the same time. The deprecation warning would still be trigger asking to change the configuration though.

alcohol · 2017-04-25T08:38:55Z

Afaik there is a BC layer in the cache plugin (see here).

Not sure what would correspond to true and false. I guess false could be [] (empty array) and true could default to the headers that the plugin uses by default: ['no-cache', 'private', 'max-age', 'no-store']?

dbu

regarding BC of the old respect_cache_headers i would suggest to just keep doing the same we did before. the BC layer is in the plugin, not in this bundle. the BC the bundle offers is that the old configuration still works.

dbu · 2017-04-25T11:06:38Z

Tests/Resources/Fixtures/config/full.xml

@@ -44,7 +44,10 @@
                <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" respect-cache-headers="false"/>
+                <config default-ttl="42" respect-cache-headers="false">
+                    <respect-response-cache-directives>X-Foo</respect-response-cache-directives>


this is supposed to raise an error: https://github.com/php-http/cache-plugin/blob/b2f15cee4ff46f61f7b90137b41627d17fd57be9/src/CachePlugin.php#L69-L72

we should remove the deprecated configuration option from the test. if you want to be precise, you can add a separate legacy test for it

alcohol · 2017-04-25T11:14:34Z

Ok I am a bit unclear on what it is that I should implement at this point.

trigger a deprecation warning?
automatically convert legacy values to new values?

.. please give me some direction :-|

stof · 2017-04-25T11:18:04Z

@alcohol if the library has a proper BC layer, here is my suggestion:

trigger a deprecation warning in the config when using the old style config (just to make it easier for people to spot where they should change something, as they don't configure the library directly, and we must force them to get rid of the old style config before the next major version of the bundle)
disallow configuring both the old style and new style config at the same time (to avoid having to merge both, which is harder to understand

Then, make sure both the new and old config styles work fine for different use cases (enabling directives, disabling them, keeping default settings)

alcohol · 2017-04-25T11:19:48Z

I'm not very familiar with the Symfony Config Definition stuff. Is it possible to specify that a node can have either A or B as a child, but not both?

fbourigault · 2017-04-25T11:32:46Z

You can look at https://github.com/php-http/HttplugBundle/blob/master/DependencyInjection/Configuration.php#L161-L167 for a similar example.

alcohol · 2017-04-25T12:04:24Z

Eh, I give up. The documentation of this config stuff is quite useless and I just keep getting different errors that provide no clear instructions as to how to fix what is wrong.

dbu · 2017-04-25T12:06:04Z

don't give up please. basically its enough to allow the old and the new configuration and trigger a deprecation warning when the old config is used. all the BC handling is done by the library. if the config can complain when both are set, all the bether. otherwise there is an exception on trying to load the service that is thrown by the plugin because it does not want both settings to be set.

alcohol · 2017-04-25T12:44:12Z

Using

                    ->arrayNode('config')
                        ->addDefaultsIfNotSet()
                        ->validate()
                        ->ifTrue(function ($config) {
                            // Cannot set both respect_cache_headers and respect_response_cache_directives
                            return isset($config['respect_cache_headers'], $config['respect_response_cache_directives']);
                        })
                        ->thenInvalid('You can\'t provide config option "respect_cache_headers" and "respect_response_cache_directives" simultaniously. Use "respect_response_cache_directives" instead.')
                        ->end()
                        ->children()
                            ->scalarNode('default_ttl')->defaultValue(0)->end()
                            ->scalarNode('respect_cache_headers')->defaultNull()->end()
                            ->variableNode('respect_response_cache_directives')
                                ->validate()
                                ->always(function ($v) {
                                    if (is_null($v) || is_array($v)) {
                                        return $v;
                                    }
                                    throw new InvalidTypeException();
                                })
                                ->end()
                            ->defaultNull()
                            ->end()
                        ->end()
                    ->end()

it adds both respect_response_cache_directives and respect_cache_headers.

Using

                    ->arrayNode('config')
                        ->addDefaultChildrenIfNoneSet(['default_ttl', 'respect_response_cache_directives'])
                        ->validate()
                        ->ifTrue(function ($config) {
                            // Cannot set both respect_cache_headers and respect_response_cache_directives
                            return isset($config['respect_cache_headers'], $config['respect_response_cache_directives']);
                        })
                        ->thenInvalid('You can\'t provide config option "respect_cache_headers" and "respect_response_cache_directives" simultaniously. Use "respect_response_cache_directives" instead.')
                        ->end()
                        ->children()
                            ->scalarNode('default_ttl')->defaultValue(0)->end()
                            ->scalarNode('respect_cache_headers')->defaultNull()->end()
                            ->variableNode('respect_response_cache_directives')
                                ->validate()
                                ->always(function ($v) {
                                    if (is_null($v) || is_array($v)) {
                                        return $v;
                                    }
                                    throw new InvalidTypeException();
                                })
                                ->end()
                            ->defaultNull()
                            ->end()
                        ->end()
                    ->end()

it throws a cryptic exception that doesn't tell me anything about how to solve this problem (neither does the documentation):

Failed asserting that exception of type "Symfony\Component\Config\Definition\Exception\InvalidDefinitionException" matches expected exception "\Symfony\Component\Config\Definition\Exception\InvalidConfigurationException". Message was: "->addDefaultChildrenIfNoneSet() is not applicable to concrete nodes at path "httplug.clients..plugins..cache.config""

stof · 2017-04-25T12:47:04Z

addDefaultChildrenIfNoneSet does not make sense on the confignode. It is only for prototyped nodes. What you need is addDefaultsIfNotSet

alcohol · 2017-04-25T12:49:32Z

@stof but that one sets all 3 configured nodes, while I only want to set 2 by default (skip the deprecated one if not provided).

stof · 2017-04-25T12:58:16Z

Well, don't put a default value on the deprecated node if it should not be there by default

alcohol · 2017-04-25T13:09:02Z

Ok I think I have the error scenario covered. Where should I do the legacy conversion and warning trigger?

stof · 2017-04-25T13:17:20Z

@alcohol don't do the conversion (the library does it). And for the warning trigger, do it in a beforeNormalization() on the old node (with ->always() as condition to trigger it, as we always want to trigger it when it appears in your config file)

alcohol · 2017-04-25T13:26:08Z

Hmm the problem I run into now is that if the deprecated respect_cache_headers config option is set (someone has it in their old legacy config), the respect_response_cache_directives config option gets added because it has a default of null. However, if I remove the default then it never sets it, regardless of whether or not the user has a legacy config or not. Is it safe to rely on the cache plugin itself to provide a default value?

stof · 2017-04-25T13:31:30Z

@alcohol I think it is.

alcohol · 2017-04-25T13:41:11Z

Okay I think that covers it, although I am not sure how to test that it actually triggers the deprecation since it gets silenced.

alcohol · 2017-04-25T13:56:27Z

Oh FFS. On Travis it fails cause the deprecation is seen. But locally (running same phpunit version) I get nothing like that.

/code/github.com/alcohol/HttplugBundle > vendor/bin/phpunit 
PHPUnit 5.7.19 by Sebastian Bergmann and contributors.

..................................................                50 / 50 (100%)

Time: 469 ms, Memory: 12.00MB

OK (50 tests, 112 assertions)

stof · 2017-04-25T13:58:25Z

Make sure you use the same versions of deps too

alcohol · 2017-04-25T14:00:04Z

For some reason I did not have the phpunit-bridge installed. Okay, lets see if I can find documentation on how to specify this deprecation as "expected".

alcohol · 2017-04-25T14:11:58Z

Good lord I wasted far too much time on this today.

dbu

thanks a lot! yeah, this can sometimes be tedious to make things clean and BC and user friendly. but you also learned some things on the configuration i guess ;-)

alcohol · 2017-04-26T05:46:16Z

Not really, I'm still not clear for example on what the difference is between a concrete node and a not-concrete node :-(. There is zero documentation on this subject.

dbu · 2017-04-26T06:41:43Z

i squashed the commits, fixed the thing that styleci complained about and merged to master.

thanks a lot for your persistence @alcohol

and i agree with you, the documentation on the configuration definition is lacking. i tend to look up examples in other bundles i know to figure out how to do things. doctrine is quite useful because they have to do some complicated use cases ;-)

alcohol · 2017-04-26T06:43:59Z

Thanks, glad I could still contribute something at least :-)

Nyholm self-requested a review April 6, 2017 13:02

Nyholm requested changes Apr 6, 2017

View reviewed changes

dbu approved these changes Apr 6, 2017

View reviewed changes

Nyholm approved these changes Apr 10, 2017

View reviewed changes

stof reviewed Apr 19, 2017

View reviewed changes

dbu reviewed Apr 25, 2017

View reviewed changes

alcohol closed this Apr 25, 2017

dbu reopened this Apr 25, 2017

dbu added the in progress label Apr 25, 2017

replace deprecated directive with new one

cf810e3

update tests, config

29a0c76

add deprecation + test

fb093bb

because just failing is not enough

b40f29c

dbu approved these changes Apr 25, 2017

View reviewed changes

dbu closed this Apr 26, 2017

dbu removed the in progress label Apr 26, 2017

alcohol deleted the replace-deprecated-directive branch April 26, 2017 06:43

replace deprecated directive with new one #145

replace deprecated directive with new one #145

Conversation

alcohol commented Apr 6, 2017

Nyholm left a comment

Choose a reason for hiding this comment

Nyholm Apr 6, 2017

Choose a reason for hiding this comment

alcohol Apr 6, 2017 • edited

Choose a reason for hiding this comment

Nyholm Apr 6, 2017

Choose a reason for hiding this comment

alcohol Apr 6, 2017

Choose a reason for hiding this comment

alcohol commented Apr 10, 2017 • edited

Nyholm commented Apr 10, 2017

alcohol commented Apr 10, 2017

Nyholm commented Apr 14, 2017

stof Apr 19, 2017

Choose a reason for hiding this comment

alcohol Apr 25, 2017

Choose a reason for hiding this comment

stof commented Apr 19, 2017

alcohol commented Apr 20, 2017

alcohol commented Apr 25, 2017 • edited

stof commented Apr 25, 2017

alcohol commented Apr 25, 2017 • edited

dbu left a comment

Choose a reason for hiding this comment

dbu Apr 25, 2017

Choose a reason for hiding this comment

alcohol commented Apr 25, 2017 • edited

stof commented Apr 25, 2017

alcohol commented Apr 25, 2017

fbourigault commented Apr 25, 2017

alcohol commented Apr 25, 2017

dbu commented Apr 25, 2017

alcohol commented Apr 25, 2017

stof commented Apr 25, 2017

alcohol commented Apr 25, 2017

stof commented Apr 25, 2017

alcohol commented Apr 25, 2017

stof commented Apr 25, 2017

alcohol commented Apr 25, 2017

stof commented Apr 25, 2017

alcohol commented Apr 25, 2017

alcohol commented Apr 25, 2017 • edited

stof commented Apr 25, 2017

alcohol commented Apr 25, 2017

alcohol commented Apr 25, 2017

dbu left a comment

Choose a reason for hiding this comment

alcohol commented Apr 26, 2017

dbu commented Apr 26, 2017

alcohol commented Apr 26, 2017

alcohol Apr 6, 2017 •

edited

alcohol commented Apr 10, 2017 •

edited

alcohol commented Apr 25, 2017 •

edited

alcohol commented Apr 25, 2017 •

edited

alcohol commented Apr 25, 2017 •

edited

alcohol commented Apr 25, 2017 •

edited