Skip to content

Conversation

@bfeaver
Copy link

@bfeaver bfeaver commented Feb 4, 2022

Adds support for the latest versions of the consul-php-sdk.

The newest version does not include the consul binary in the package. I tried to see if there was anything that prevented v3 or v4 from being used, and all tests passed. Was there something that prevented using anything above v2?

@eli-darkly
Copy link
Contributor

Was there something that prevented using anything above v2?

I'm not aware of anything specific; possibly we just had not done retesting since the release of the later major versions. Generally we would not want to have a completely open-ended dependency, because it's always possible (even likely) that a new major version will not be API-compatible and we don't want existing applications that use our SDK to break simply because a third-party project had a new release. So we normally have them pinned at least to a major version, as we did here, and we may just have neglected to update it.

If our own testing confirms that both v3 and v4 are fine, then this is a reasonable change.

@codayblue
Copy link

Any way I can assist in getting this through?

@bfeaver
Copy link
Author

bfeaver commented Mar 18, 2022

Ran into an interesting issue today that this change will solve. Your current php-sdk and the consul-php-sdk don't play nicely with Guzzle. The 2.1.0 version of the consul-php-sdk requires Guzzle ^6.0. However, your Guzzle feature requester in the main php-sdk requires Guzzle 7. I was able to get around it using our branch instead.

@eli-darkly eli-darkly self-requested a review June 14, 2022 19:53
@eli-darkly
Copy link
Contributor

I apologize for letting this PR fall through the cracks - I had not configured reminders to keep it on our radar, and I somehow missed the comments from March.

Looking at it again now, I think it would definitely be OK to enable usage with consul-php-sdk 4.x. I'm less sure about whether we can still retain support for 2.x and 3.x, because the Guzzle compatibility issue that @bfeaver mentioned applies to both of those. In other words, because of the Guzzle 7.x version requirement of the main SDK package, I believe you will always end up with Guzzle 7.x and as a result you will at a minimum get a bunch of deprecation warnings due to consul-php-sdk trying to use older APIs. So I'm wondering if the right way to go here would be to release a new major version of our php-server-sdk-consul package which requires specifically consul-php-sdk 4.x.

@eli-darkly
Copy link
Contributor

I think that is actually the right way to go— that is, I don't think it would work to have a single version of this package that can work with consul-php-sdk 2.x and 3.x and 4.x, because those versions do not have the same dependency requirements. If we want to extend the current version to support 2.x and 3.x, that's a separate decision, but I think we should bump this package to a new major version for 4.x support.

@eli-darkly
Copy link
Contributor

@bfeaver @codayblue We've released a new version of this package that works with version 4.x of consul-php-sdk. Sorry again for the delay.

There is also a 5.x version of the Consul client now, but we definitely can't include that one in the compatibility range for this same release of our package, because they made breaking changes to the package name and the namespace. So we'll be putting out another release for that one.

@eli-darkly eli-darkly closed this Jun 15, 2022
@eli-darkly
Copy link
Contributor

We've now also put out a 3.0.0 version of this integration that works with friendsofphp/consul-php-sdk 5.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants