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

Provider dependencies detection #61

Open
cedricalfonsi opened this issue Mar 7, 2016 · 8 comments
Open

Provider dependencies detection #61

cedricalfonsi opened this issue Mar 7, 2016 · 8 comments

Comments

@cedricalfonsi
Copy link

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 : #60

Let me know if it makes sense to you

@ThaDafinser
Copy link
Owner

@cedricalfonsi the check was done that way when i started with the package (but it was not testable)

if (! function_exists('parse_user_agent')) {
throw new Exception\PackageNotLoadedException('You need to install ' . $this->getComposerPackageName() . ' to use this provider');
}

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
Copy link
Owner

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

I'll close the PR in the meantime

@cedricalfonsi
Copy link
Author

@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.

@ThaDafinser
Copy link
Owner

@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_

@cedricalfonsi
Copy link
Author

@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.

@ThaDafinser
Copy link
Owner

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

@ThaDafinser ThaDafinser reopened this Mar 20, 2016
@ThaDafinser ThaDafinser added bug and removed wontfix labels Mar 21, 2016
@cedricalfonsi
Copy link
Author

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

@ThaDafinser
Copy link
Owner

@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

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

No branches or pull requests

2 participants