UserAgentParser icon indicating copy to clipboard operation
UserAgentParser copied to clipboard

Provider dependencies detection

Open cedricalfonsi opened this issue 9 years ago • 8 comments

In order to be able to deploy the library in various environments, it would be better to check if the primitive functions/classes of the different providers are present instead of checking if the package has been deployed in a specific vendor directory.

Even if it's a best practice to put external libraries in a vendor directory, all projects don't respect it and can't deploy your great library.

I created a pull request for the DonatjUAParser Provider : https://github.com/ThaDafinser/UserAgentParser/pull/60

Let me know if it makes sense to you

cedricalfonsi avatar Mar 07 '16 09:03 cedricalfonsi

@cedricalfonsi the check was done that way when i started with the package (but it was not testable) https://github.com/ThaDafinser/UserAgentParser/blob/8318722a977b89f66c4d6b9b7bc4a013875f8449/src/Provider/DonatjUAParser.php#L43-L45

But since this package only supports composer for installation, i wasn't seeing a problem with it (expect the performance of a single file call)

In the next major version (which only support PHP7), the check will be hopefully done by this awesome package https://github.com/Ocramius/PackageVersions#package-versions

@cedricalfonsi if you find a (clean) way to test function_exists() we can probably change it. Otherwise i suggest you to uncomment that check by hand

ThaDafinser avatar Mar 07 '16 10:03 ThaDafinser

@cedricalfonsi any idea? I'm pretty sure we could do this better somehow

I'll close the PR in the meantime

ThaDafinser avatar Mar 10 '16 12:03 ThaDafinser

@ThaDafinser as composer is required to install the lib it makes sense.

I checked the composer API without any success. But we can do something by our own, knowing that the lib will be installed itself in the vendor-dir, we can find it easily.

in AbstractProvider.php

    /**
     * Get vendor dir
     *
     * @return string
     */
    protected function getVendorDir() {
        return dirname(dirname(dirname(dirname(__DIR__))));
    }

and in the providers constructs, by instance here for DonatjUAParser

    public function __construct()
    {
        if (! file_exists($this->getVendorDir() . '/' . $this->getPackageName() . '/composer.json')) {
            throw new PackageNotLoadedException('You need to install the package ' . $this->getPackageName() . ' to use this provider');
        }
    }

What do you think about that ? If ok I'll send a PR.

cedricalfonsi avatar Mar 10 '16 14:03 cedricalfonsi

@cedricalfonsi i was thinking about this a while (that's why i reply so late, sorry about that)

By doing dirname(dirname(dirname(dirname(__DIR__)))) we limit ourself again to a fixed structure. So that will work for no composer installation in some cases, but not for composer installation anymore.

There are two possible solutions and ways after them

  • check if a package is installed over function_exists() or class_exists()
    • package can be used by none composer users
    • hard to test / mock
  • leave the current file_exists()check and do it later maybe with ocramius/package-versions package
    • only composer is supported
    • easy to test

Since i know currently no reason to not use composer, i'll leave it as is for now. If someone wants to use "modern" software, a package manager is required anyway - since the dependency cant be handled manually anymore.

_Still want to use the package without composer? Comment out the check in the __construct() method_

ThaDafinser avatar Mar 18 '16 13:03 ThaDafinser

@ThaDafinser Actually, we also use composer in our project, but the vendor-dir has a different name, that's why we need to locate it.

dirname(dirname(dirname(dirname(__DIR__)))) does the job, as the structure of the UserAgentParser is well known.

What do you mean by it "limit ourself again to a fixed structure" ? If the structure of the UserAgentParser lib will change, we will also change the getVendorDir function.

We are few in this case we renamed the vendor-dir of composer, and I agree it will be more simple to manage this case with future lib such as the one you mentioned.

cedricalfonsi avatar Mar 20 '16 10:03 cedricalfonsi

Now i get you. You use this option? https://getcomposer.org/doc/03-cli.md#composer-vendor-dir

ThaDafinser avatar Mar 20 '16 17:03 ThaDafinser

@ThaDafinser Yes exactly, I should have mentioned it more clearly in my first message.

cedricalfonsi avatar Mar 22 '16 10:03 cedricalfonsi

@cedricalfonsi i think we implement this in 2.0.

If we change the code to your variant dirname(dirname(dirname(dirname(__DIR__)))) we would also need a check if the folder exists, since that wont work if you run the tests in the project itself and we would need here another case too.

So for now, the check must be disabled until v2.0

ThaDafinser avatar Apr 04 '16 13:04 ThaDafinser