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

Recommended way to determine Guzzle 7? #2579

Closed
bshaffer opened this issue Feb 3, 2020 · 12 comments · Fixed by #2583
Closed

Recommended way to determine Guzzle 7? #2579

bshaffer opened this issue Feb 3, 2020 · 12 comments · Fixed by #2583

Comments

@bshaffer
Copy link

bshaffer commented Feb 3, 2020

Description
For any library compatible with multiple versions of Guzzle, we no longer have a way to reliably determine the version in Guzzle 7, as the ClientInterface::VERSION constant has been removed. What is the recommended way to tell that Guzzle 7 is installed?

Currently we assume Guzzle 7 if the VERSION constant is not defined, but this will break with Guzzle 8. I've noticed in the PRs that @Nyholm is concerned with breaking BC every time a new release comes out, but if only the major version is captured in the constant, or if the version is specified in a method instead of a constant (e.g. Client::getGuzzleVersion()), this would not be the case.

An even better option would be to simply include a VERSION file in the root of this repo with the version number (like we do here)

Example

GuzzleHttp\Client::getGuzzleVersion()

Additional context
https://github.com/googleapis/google-auth-library-php/pull/256/files

@GrahamCampbell
Copy link
Member

You can get the version from composer. It will tell you.

@GrahamCampbell
Copy link
Member

<?php

class Example
{
    public static function getGuzzleVersion()
    {
        static $version;

        if ($version === null) {
            $packages = @json_decode(file_get_contents(self::getComposerInstalledFile()), true);

            if (!is_array($packages)) {
                throw new RuntimeException('Unable to obtain Guzzle version from Composer.');
            }

            foreach ($packages as $package) {
                if ($package['name'] === 'guzzlehttp/guzzle') {
                    $version = $package['version'];

                    break;
                }

                throw new RuntimeException('Unable to obtain Guzzle version from Composer.');
            }
        }

        return $version;
    }

    private static function getComposerInstalledFile()
    {
        // UPDATE THIS RELATIVE TO WHERE YOU PUT THIS CODE IN YOUR LIBRARY
        // IN THIS EXAMPLE, I HAVE ASSUMED YOU PUT THIS "Example" CLASS DIRECTLY
        // IN A "src" FOLDER, SO IT IS INSTALLED AS "vendor/your/library/src/Example.php"
        return __DIR__.'/../../../composer/installed.json';
    }
}

@Nyholm
Copy link
Member

Nyholm commented Feb 3, 2020

I think it is a bad idea to have the full version as a constant. It is one thing that is super easy to forget when releasing a new version since we have not scripted released. It is also technically a BC break.

But to make things easier, we can add a MAJOR_VERSION constant to both Guzzle 6 and Guzzle 7.
Would that be a good and easy solution?

@sagikazarmark
Copy link
Member

Personally, I think the best thing you can do is not depend on Guzzle at all. You should depend on PSR-18 instead and leave choosing a client to the consumer.

Otherwise a MAJOR_VERSION constant sounds like the easiest solution, but TBH, I'm not a huge fan of it.

@bshaffer
Copy link
Author

@GrahamCampbell the way you suggest relies on a composer.json being present, which may not be the case since it may not be deployed in production. Additionally, it's a fairly complicated and inefficient way to go about it, requiring a file read and JSON parsing.

@Nyholm Yes, having a MAJOR_VERSION would be exactly what we're looking for. But @sagikazarmark makes a good point that we may be able to just handle Guzzle 5 vs PSR-18, and otherwise not care.

@GrahamCampbell
Copy link
Member

@GrahamCampbell the way you suggest relies on a composer.json being present, which may not be the case since it may not be deployed in production.

No, it doesn;t use the composer.json. It uses the special file within the vendor folder which tells you what packages are there.

@GrahamCampbell
Copy link
Member

I would definitely say a major version constant is a better idea than recommending people do a hack like I have provided to grab the installed package version. :P

@bshaffer
Copy link
Author

@GrahamCampbell Ahh, roger that. Thanks for the clarification. I still do not think it wise to depend on a composer config file and require a file read + json parse.

@gmponos
Copy link
Member

gmponos commented Feb 12, 2020

@Nyholm
Copy link
Member

Nyholm commented Feb 12, 2020

It's a hack I did, it is not perfect :/

@bshaffer
Copy link
Author

@Nyholm Aha!! So you DO need it too!! 😄

@Nyholm
Copy link
Member

Nyholm commented Feb 13, 2020

I decided not to add it to 6.x because it would not be a reliable way to determine version.

See #2583

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 a pull request may close this issue.

5 participants