Allow to use environment variables for timeouts

[edefimov]

Q	A
Bug fix	no
New feature	yes
BC breaks	no
Deprecations	no
Tests pass	yes
Fixed tickets
License	MIT

This PR adds read_timeout option for guzzle client and allows using environment variables for connect_timeout, read_timeout and timeout settings.

[gregurco]

@edefimov as I understand, the problem is that env's are strings, but bundle accepts only floats for timeout? Why not to use type casting for env variables introduced in 3.4?

Doc: https://symfony.com/blog/new-in-symfony-3-4-advanced-environment-variables Example: '%env(float:TIME_OUT)%'

[edefimov]

Changed example value in the tests, but in configuration we still need to keep values as scalarNode

[gregurco]

@edefimov why? why we can't use type hinting for env variables?

[edefimov]

@gregurco During building a container environment variables are represented as strings, so validation for floatNode will fail. Or do you mean to add something like

    ->scalarNode('connect_timeout')->example('%env(float:CONNECT_TIMEOUT)%')->end()

into Configuration.php?

[edefimov]

@gregurco, I've tested this config with floatNode

eight_points_guzzle:
  clients:
    my_client:
      options:
        connect_timeout: "%env(float:MY_CLIENT_CONNECT_TIMEOUT)%"

The execution result is:

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  In FloatNode.php line 34:
!!                                                                                 
!!    Invalid type for path "eight_points_guzzle.clients.my_client.options.connect  
!!    _timeout". Expected float, but got string.                                   
!!                                                                                 
!!  
!!  

because of this check, - it doesn't work, when parameter for the node is environment variable, see symfony/symfony#25868.

So the solution is using a scalarNode with type-casting numeric strings to float values. What do you think?

[gregurco]

@edefimov As I understood, it works, but the flow is a little bit more difficult. What happens when symfony build cache:

1. container builder is created (not container)

2. all parameters are collected

   • if it's env parameter, it's replaces by placeholder, not by real value. For example, we will have env_int_OUR_ENV_VAR_NAME_b1c350cd095612a9b4d66298cd146978 on this step but not our real env value (ref: https://github.com/symfony/dependency-injection/blob/master/ParameterBag/EnvPlaceholderParameterBag.php#L30)

3. all services are collected

4. all bundles are collected

5. container build is resolved and compiled and we receive container

   • on this step placeholders of env parameters are replaced by real values (ref: https://github.com/symfony/dependency-injection/blob/master/ContainerBuilder.php#L1454)

6. we use container in our app

So, the problem is that we try to receive real float/int value on the step (4) when there is placeholder (string) but not real value and one of the options is to write:

->variableNode('connect_timeout')
    ->validate()
        ->ifTrue(function ($v) {
            return !is_numeric($v) && !(is_string($v) && strpos($v, 'env_') === 0);
        })
        ->thenInvalid('connect_timeout can be: float')
    ->end()
->end()

But in this case user can pass anything through env variable.

[edefimov]

I think it would be better to use such code for timeouts:

    ->scalarNode('connect_timeout')
        ->example('%env(float:CONNECT_TIMEOUT)%')
        ->beforeNormalization()
            ->always(function ($v) {
                return is_numeric($v) ? (float) $v : $v;
            })
        ->end()
        ->validate()
            ->ifTrue(function ($v) {
                return !is_float($v) && !(is_string($v) && strpos($v, 'env_') === 0);
            })
            ->thenInvalid('connect_timeout can be: float')
        ->end()
    ->end()

We still keep the behaviour of floatNode with casting to real floats and solve the problem of passing environment variables.

[gregurco]

@edefimov for me it's ok. Only remove examples and I will approve it. @florianpreusner what do you think?

[gregurco]

I suggest to wait until https://github.com/symfony/symfony/pull/23888 will be merged and to check the problem in context of the fix.

[florianpreusner]

Implementation looks good to me. I think symfony/symfony#23888 could take some more time. Seems that they still don't have any clean solution... Why would you like to wait, @gregurco?

[gregurco]

@florianpreusner yep, probably it will take some time and there is a chance that new behaviour will be merged in 4.1, not in 4.0.x... Ok, let's merge. I will track symfony/symfony#23888 and will do changes if it will be required.

@edefimov thank you for contribution and your patience :slightly_smiling_face: