Laravel 8 compatibility

[ngunyimacharia]

Update guzzlehttp version to allow Laravel 8 compatibility. Laravel 8 requires "guzzlehttp/guzzle": "^7.0.1"

[scorgn]

Would like to see this merged as well as this is one of the last two packages holding me back from updating our services to Laravel 8

[scorgn]

I'm sorry I am mistaken, only the root package 's dev dependencies are installed so you wouldn't need to update that in composer.json. You might want to revert the merge with my fork since there may be breaking changes in 9.

[ngunyimacharia]

I'm sorry I am mistaken, only the root package's dev dependencies are installed so you wouldn't need to update that in composer.json. You might want to revert the merge with my fork since there may be breaking changes in 9.

Thank you for pointing this out.

[scorgn]

Reading the issues it looks like they're heading in a direction that would allow for any PSR-18 implementation (which includes Guzzle 7). It wouldn't make sense to switch the dependency to Guzzle 7 which would require a major version upgrade. I am going to put a PR in soon which switches things over to depend on a PSR-18 implementation.

[mghoneimy]

While the changes in this PR look good, I'm conflicted about merging it. Not sure if stopping support for PHP7.1 in a minor version bump would violate semantic versioning.
While this change does not break code backward compatibility, it will prevent some of the existing package users (PHP7.1 users) from being able to update to the latest minor release.
The only reason I'd think this would be ok is that PHP7.1 is well past its end of life date. In fact, PHP7.2 end of life is approaching by the end of the year.

Would like to hear your thoughts on this.

[scorgn]

I assumed that because Guzzle 7 has backwards compatibility breaking changes that requiring it would make it so that the library would as well. But that might not be the case, I'm not sure. If it can be done with a minor version upgrade I don't see much of a reason of not merging the changes.

Edit: Could we potentially release this with "guzzle/http":"^6.0|^7.0.1", to allow compatibility with Guzzle 7 but not force it? We could keep the PHP version as ^7.1 in that case and get this out of the door.

[mghoneimy]

The tests are passing without making any code changes and I reviewed the backwards compatibility breaking changes they introduced in Guzzle 7 (https://github.com/guzzle/guzzle/blob/master/UPGRADING.md), none of them is being used in the package.

@ShawnCorrigan I like this suggestion! This solves the Laravel 8 problem without changing the minimum PHP version.

Can someone apply this change please?
I will merge it straight away

[scorgn]

I am not able to make changes but I put suggestions for @ngunyimacharia. If nothing else by the end of the day I can make a PR with the changes.

On an unrelated note, if a package is using another package that has breaking changes that could effect the end user, but doesn't actually effect the package itself, would that be considered a breaking change in the first package? I'm not too familiar with semantic versioning so I'm not sure.

As an example, the package doesn't ever interact with or reference the ConnectException class in Guzzle, but if the end user were to be catching the ConnectException and calling the hasResponse() method on it then that method would no longer be available on the exception. Would that be considered a non backwards-incompatible change?

The scenario is very unlikely as the method would always return false so there is no reason to call it, so I don't think it's a concern for this specific update. But I'm just wondering in general what that would be considered as.

[mghoneimy]

@ShawnCorrigan That's a very good question.The answer is tricky, and relative depending on who you ask in my opinion. This is how i understand this:
If the package exposes some of its dependencies objects (be it classes or exceptions) then the dependency version becomes part of the package version. As you explained in your example, introducing backwards compatibility breaking to the exposed dependency version would in turn potentially introduce backwards compatibility breaking changes to the package end user and breaking their logic.
If the package on the other hand completely encapsulates and abstract all its dependencies objects, then introducing backwards incompatible changes shouldn't be an issue since the package end user will never be aware that such a change has happened.
That scenario though might still be troublesome for the package end user as they might be using the dependency for another purpose, for example, you would expect a PHP application to be using Guzzle for making HTTP requests whether it's using this package or not, and making backwards incompatible change to this dependency would lead to a dependency resolution issue at the end user's side side. This does not break semantic versioning, but it's just troublesome for package users.

It's generally common wisdom not to bump up a dependency major version in a minor version release, unless someone really knows what they are doing.

For this package's case, Guzzle's object are abstracted away from the end user. The requests are requests internally and responses are handled adapted into a custom object before being returned to the users. The exceptions though are not. We are only catching the ClientException, leaving all other Guzzle exceptions to get propagated to the end user, which might potentially cause an issue if the exceptions change significantly and someone is using them in their logic.

For the sake of this release, I think it would be ok given the small scale of the change, the limited Guzzle exposed objects by the package, and the fact that we'll be providing the end user with more than one option for installing Guzzle depending on their project's dependencies.

Managing dependencies is fun 😄

[mghoneimy]

On a side note, let's give @ngunyimacharia until the weekend, this is not super urgent.

[ngunyimacharia]

I've accepted the suggested changes.

[mghoneimy]

@ngunyimacharia I'm ready to merge this now. One important question before I do though, were you able to verify that this works with Laravel 8?
I know that it should, but I haven't tried it myself, wondering if you did.

Thanks

[ngunyimacharia]

I haven't tried it as well. The integration we were using this for is no-longer active in our project.

[mghoneimy]

You don't need to run any code, just verify that installation works well on Laravel 8.

I just pushed your changes as a dev branch to my composer package to make your life easier.

Start a new Laravel8 project, then try this:
composer require gmostafa/php-graphql-client
This should fail because of guzzle 6 dependency.

Now try this:
composer require gmostafa/php-graphql-client:dev-laravel8
This should succeed because of adding guzzle 7 as an options in composer.json file.

If this installs successfully then the changes are compatible with Laravel8 and we can merge this pull request.

[ngunyimacharia]

I can confirm, it installs successfully.

[mghoneimy]

Thank you @ngunyimacharia and @ShawnCorrigan for your contribution!

[lucascardial]

So many thanks!