Add conflict with some guzzle versions

[franmomu]

Closes #2065

The thing is that we are testing "guzzlehttp/guzzle": "^6.3 || ^7.2", but a user right now could for example use version 4 and will probably get some errors.

Adding a conflict allow us to be able to restrict which versions of the library we are supporting.

[ruflin]

Thanks for the PR, this makes it much simpler to discuss.

One thing I'm curious: Lets assume someone uses Elastica and a guzzle version 4.0. But all the features this users is using in Elastica don't use guzzle. Now a user upgrading Elastica would suddenly have a theoretical conflict but none in practice?

[ruflin]

Was this removed on purpose?

[franmomu]

yeah, I did it in a separated commit 278ce0d (the project is abandoned), but it could be done in another PR.

[deguif]

This was deprecated in #1940 so stopping to suggest it is a logical move I think

[ruflin]

Removal SGTM, can we move it to a separate PR? Sounds like it is not related to the guzzle discussion.

[franmomu]

One thing I'm curious: Lets assume someone uses Elastica and a guzzle version 4.0. But all the features this users is using in Elastica don't use guzzle. Now a user upgrading Elastica would suddenly have a theoretical conflict but none in practice?

Yeah, that's the drawback of this approach 😞 I guess that is somehow reasonable if the range of versions makes sense, for example guzzle 6.3 was released 5 years ago, so I guess that having a lower bound of 6.3 should be fine for most people.

That's the thing with optional dependencies, I guess the best would be to have another package like elastica-guzzle or elastica-aws, but that comes with more maintenance work... which is not always desirable.

(It needs a changelog, I'll add it if we think this is right.)

[deguif]

One thing I'm curious: Lets assume someone uses Elastica and a guzzle version 4.0. But all the features this users is using in Elastica don't use guzzle. Now a user upgrading Elastica would suddenly have a theoretical conflict but none in practice?

Yes, you're absolutely right, maybe to avoid this case, we could do some runtime check and throw an exception at runtime instead of adding conflict in dependencies, WDYT?

[ruflin]

Thanks for the additional details. The conflict sounds too intrusive to me. We create pain for some users that would be fine using it the way it is by trying to protect some other users from shooting themself into the foot, at least this is my understanding.

The runtime check proposed by @deguif sounds less intrusive.

But then I still stumble over:

for example guzzle 6.3 was released 5 years ago, so I guess that having a lower bound of 6.3 should be fine for most people.

Why do we need this check in the first place? Are we trying to fix an actual issue or are we introducing additional checks for a theoretical edge case? @franmomu Can you share a bit more background here?

[franmomu]

The idea behind this (it started in #2064 (review)) was to be able to add and remove compatibility with guzzle versions without creating BC breaks (in minor versions).

At some point we would want to remove support of guzzle 6, to do so I think the only way right now is to create a major version.

Adding a conflict at composer level allow us to prevent/allow users to only use the supported versions of guzzle (with the drawback commented in #2066 (review)) and change those versions in a minor release.

A runtime check it's also valid in optional dependencies with the drawback that we should keep compatibility with old versions of guzzle until we release a major version.

At the end is about trade-offs.

[ruflin]

As you stated @franmomu , its all about tradeoffs.

guzzle is not a required but a suggested dependency. Breaking some users setup for a suggested dependency does not sound great even if the lib the users are using is very outdated.

What holds me back from doing any change at all is that it is not an issue that was reported so far or at least I have not seen it. Even the runtime check is nice, but do we need the additional code?

There is still the question around updating the dependencies only for majors. Good news is that we have a major release upcoming :-) At the same time I wonder if might be able to get rid of the suggested dependency to get rid of the problem?

[franmomu]

I don't have enough knowledge of this package, but maybe we could use theClient from elastic/elasticsearch-php in next major version (I guess it would require quite work and BC breaks)

[ruflin]

There are some different opinions around using elastic/elasticsearch-php as the client. I'm personally all in favour of using it and internally there are more and more places where it is used.