Could the Guzzle/Redis dependencies be always included?

[halfer]

This is either a suggestion for a future release, or a question to understand why dependencies for this library have been arranged in the way they have.

It seems that implementers have the choice of using either Guzzle or Redis to communicate with LD, and that they should install libraries accordingly. I wonder if it would make it easier to bundle all of those libraries, and then supply options to select the one to use? In a code review we found the Guzzle/Middleware libraries in our composer.json and it took us a while to work out why they were there. :smile_cat:

The only reason I can think of not to do this is if the implementer wishes to use a slightly different version of Guzzle or Predis, though I imagine that can be solved by having a wide allowance in the LD dependency allowed versions.

[pierswarmers]

Yep, would love to see this.

[drichelson]

Thanks for the thoughts @halfer and @pierswarmers! We've chosen only to bundle dependencies that are required for all possible configuration options and then allow you to include any extras that you may need. If there's a way to specify various sets of dependencies (similar to the maven artifact classifier), then ideally we should do that, but I'm not aware of such a mechanism in composer. The downside of bringing in extra dependencies is that they are very often not needed and can cause unexpected side effects.

We're open to any ideas that allow for more flexible transitive dependency management, but bringing in both Predis and Guzzle doesn't seem like the right way to go.

[pierswarmers]

Thanks for the clarification @drichelson. Really like the idea of the configurable FeatureRequesters. I think one of the reasons there is confusion here is the way the dependancy injection is implemented with the $options. IMHO, it can make it confusing to users.

If the client has a dependancy on a feature requester, then I believe there should be a contract for that upfront. This feels a bit weird:

    private function getFeatureRequester(array $options, $sdkKey)
    {
        if (isset($options['feature_requester']) && $options['feature_requester'] instanceof FeatureRequester) {
            return $options['feature_requester'];
        }

        if (isset($options['feature_requester_class'])) {
            $featureRequesterClass = $options['feature_requester_class'];
        } else {
            $featureRequesterClass = GuzzleFeatureRequester::class;
        }

        if (!is_a($featureRequesterClass, FeatureRequester::class, true)) {
            throw new \InvalidArgumentException;
        }
        return new $featureRequesterClass($this->_baseUri, $sdkKey, $options);
    }

Unless I missed something, the Client always needs a feature requester. It would seem neater to provide the client with a requester:

public function __construct($sdkKey, FeatureRequester $featureRequest, $options = array()) { … }

Straight away the dependency is defined.

[arun251]

This is an interesting idea @pierswarmers . I'm curious -- do you use PHP-DI or any other DI framework?

I suspect we would need to make this change in a major release. I'm inclined to create a new issue for that change and close this issue.

[arun251]

I'm closing this issue due to timeout and that it may be addressed by #78. However, I welcome any additional discussion.

[halfer]

I'm no longer working on the project that prompted this ticket, so were any changes to be made, I would not be in a position to benefit.

The original problem was really just about discovering the purpose of dependencies I didn't recognise, and it's unfortunate that the Composer JSON spec does not allow comments, which would have solved that :-p