Add setter to adjust the http clients request timeout

[vntw]

This PR introduces a setRequestTimeout method to the TheIconic\Tracking\GoogleAnalytics\Network\HttpClient class to adjust the hard-coded request timeout that is passed to the Guzzle client. 100s seems excessive to me, so I'd like to change this value without overwriting the entire client.

WDYT?

[coveralls]

Coverage remained the same at 100.0% when pulling d5d55a74b75bd24f6e5d54b2c6b31539a5d48dd9 on venyii:adjustable-req-timeout into 6b9d0a36b5e50e9d134508ccd7e8d6d404bc91e5 on theiconic:master.

[coveralls]

Coverage remained the same at 100.0% when pulling d5d55a74b75bd24f6e5d54b2c6b31539a5d48dd9 on venyii:adjustable-req-timeout into 6b9d0a36b5e50e9d134508ccd7e8d6d404bc91e5 on theiconic:master.

[vntw]

@jorgeborges thanks for wanting to review this, do you have time to do this in the near future?

[jorgeborges]

@venyii Sorry for taking so long, I'll try to review in detail this weekend.

Can you justify why you want to add this value directly in the HttpClient class? This class is not meant to be used by the user, its encapsulated in the library. Notice all the methods have the @internal tag.

I think the best way to do this is by creating a third parameter in the Analytics class constructor called array $options and inside this array have users add a key-value called 'timeout'.

Example:

class Analytics
...
public function __construct($isSsl = false, $isDisabled = false, array $options = [])

And then to instantiate this you would have to do for example:

$analytics = new Analytics(false, false, ['timeout' => 100])

$isSsl and $isDisabled should be inside $options too, but I'll implement that breaking change in v3 to not break anyone's code currently in production.

[vntw]

Hi, thanks for the quick reply! No particular reason why I added it to the HttpClient, I don't mind moving the logic elsewhere.

Thanks for the proposal, how would you handle this within the HttpClient? Also add a dedicated options array since you'd rather not want to expose setters? If options array, I'd assume you'd also want to put nonBlocking into an options array.

Analytics::sendHit()

        [...]

        if ($this->isDisabled) {
            return new NullAnalyticsResponse();
        }

        $requestOptions = ['async' => $this->isAsyncRequest];

        if (isset($this->options['timeout'])) {
            $requestOptions['timeout'] = $this->options['timeout'];
        }

        return $this->getHttpClient()->post($this->getUrl(), $requestOptions);
}

HttpClient

    public function post($url, array $options = [])
    {
        $request = new Request(
            'GET',
            $url,
            ['User-Agent' => self::PHP_GA_MEASUREMENT_PROTOCOL_USER_AGENT]
        );

        $opts = $this->parseOptions($options);
        $response = $this->getClient()->sendAsync($request, [
            'synchronous' => !$opts['async'],
            'timeout' => $opts['timeout'],
            'connect_timeout' => $opts['timeout'],
        ]);

        if ($opts['async']) {
            self::$promises[] = $response;
        } else {
            $response = $response->wait();
        }

        return $this->getAnalyticsResponse($request, $response);
    }

    /**
     * Parse the given options and fill missing fields with default values.
     * 
     * @param array $options
     * @return array
     */
    private function parseOptions(array $options)
    {
        $defaultOptions = [
            'timeout' => static::REQUEST_TIMEOUT_SECONDS,
            'async' => false,
        ];

        $opts = [];
        foreach ($defaultOptions as $option => $value) {
            $opts[$option] = isset($options[$option]) ? $options[$option] : $defaultOptions[$option];
        }

        if (!is_int($options['timeout']) || $options['timeout'] <= 0) {
            throw new \UnexpectedValueException('The timeout must be an integer with a value greater than 0');
        }

        if (!is_bool($options['async'])) {
            throw new \UnexpectedValueException('The async option must be boolean');
        }

        return $opts;
    }

I'd just like to know how the HttpClient should handle this, then I'll make the change! What do you think?

Thanks!

[jorgeborges]

@venyii That looks awesome, I'm OK with this implementation. Remember to update the unit tests please.

[coveralls]

Coverage remained the same at 100.0% when pulling cc2f9f68ff08e9d6a01da6b84e697dd7d4986e7b on venyii:adjustable-req-timeout into 6b9d0a36b5e50e9d134508ccd7e8d6d404bc91e5 on theiconic:master.

[coveralls]

Coverage remained the same at 100.0% when pulling cc2f9f68ff08e9d6a01da6b84e697dd7d4986e7b on venyii:adjustable-req-timeout into 6b9d0a36b5e50e9d134508ccd7e8d6d404bc91e5 on theiconic:master.

[coveralls]

Coverage remained the same at 100.0% when pulling cc2f9f68ff08e9d6a01da6b84e697dd7d4986e7b on venyii:adjustable-req-timeout into 6b9d0a36b5e50e9d134508ccd7e8d6d404bc91e5 on theiconic:master.

[vntw]

Thanks for the info, I made the changes accordingly 👍

[jorgeborges]

Thanks @venyii, I'll review again as soon as I got the chance.