Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.0] Avoid functions due to global install conflicts #2546

Merged
merged 1 commit into from Jan 19, 2020
Merged

[7.0] Avoid functions due to global install conflicts #2546

merged 1 commit into from Jan 19, 2020

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Jan 11, 2020

Don't call the GuzzleHttp "functions"; instead use class static methods. The functions themselves could be marked as deprecated in 7.1.0.


This is the full version of #2548, which was only applied to the new internal functions added on or after 6.4.0, which are directly causing problems for people.

@GrahamCampbell GrahamCampbell changed the title [7.0] Don't call the GuzzleHttp "functions"; instead use class static methods [7.0] Avoid functions due to global install conflicts Jan 13, 2020
@Nyholm
Copy link
Member

Nyholm commented Jan 17, 2020

I like this PR. But you have kept the functions. That means there could still be conflicts, right?

@GrahamCampbell
Copy link
Member Author

I kept them to make sure that upgrading to guzzle 7 is easy. We should deprecate in 7.1.0, and remove in 8.0.0, perhaps?

@GrahamCampbell
Copy link
Member Author

There will be no conflicts within the internals, at least, so upgrading to Guzzle 7 won't result in Guzzle 7 calling globally installed guzzle 6 functions.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

use GuzzleHttp\Handler\StreamHandler;
use Psr\Http\Message\UriInterface;

final class Utils
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please me more defensive here.

Mark it as @internal and write a comment to explain the intent of the class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this class is internal only? We do intend for people to call these class methods, instead of the functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, It is just me that want to get rid of the functions AND eventually this class.

We can discuss that in a separate PR or issue.

@Nyholm Nyholm added this to the 7.0.0 milestone Jan 18, 2020
@Nyholm Nyholm merged commit 257ec50 into guzzle:master Jan 19, 2020
@GrahamCampbell GrahamCampbell deleted the functions branch January 19, 2020 08:16
@GrahamCampbell
Copy link
Member Author

@Nyholm I also have a lighter version of this PR for 6.5.x, which only moves the internal-marked functions to a Utils class: #2548. In particular, this fixes problems when an older version of 6.x is installed globally, but 6.5.x is installed locally.

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.

None yet

2 participants