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

[5.5] Autoload Package Providers #19420

Merged
merged 8 commits into from
Jun 1, 2017
Merged

[5.5] Autoload Package Providers #19420

merged 8 commits into from
Jun 1, 2017

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented May 31, 2017

Note for anyone who comes across this PR: the final implementation differs from the proposal below. Please check in the docs how auto-discovery works when 5.5 is released.

This PR adds autoloading functionality for packages. Package maintainers can define their providers through the extra key in their composer.json file and they will be automatically loaded by the framework, eliminating the need for developers to manually register them.

Example:

{
    "extra": {
        "providers": "MyPackage\\MyServiceProvider"
    }
}

Or define multiple providers:

{
    "extra": {
        "providers": [
            "MyPackage\\MyFirstServiceProvider",
            "MyPackage\\MySecondServiceProvider",
        ]
    }
}

Benefits

  • Finally development related packages with service providers don't need to be registered anymore in the require list. They may simple be added in require-dev and when deploying to production, they won't be loaded into the app anymore
  • Installation for packages are done automatically, eliminating the need for developers to manually set things up

Some Notes

  • Functionality is on by default but can be opted out through a new config option
  • Package providers need to be known up front so they're cached in a new packages.php file in bootstrap/cache
  • The new PackageAssetLoader accepts any given key so you can still use it for other keys than just providers

@RamonSmit
Copy link

I like this idea 👍

@m1guelpf
Copy link
Contributor

m1guelpf commented May 31, 2017

@driesvints Can't 👍 enough!
Also, would this work with the existing cache artisan commands? (I mean, if I cache everything on deployment, would thisstill work?)

@driesvints
Copy link
Member Author

@m1guelpf yeah, this falls outside the caching commands. It'll create a new packages.php file in bootstrap/cache that's being handled the same way as the services.php file.

@m1guelpf
Copy link
Contributor

m1guelpf commented Jun 1, 2017

@driesvints And wouldn't this broke everything if a non-existent provider is provided? Is there some thind of check for this before caching?

@driesvints
Copy link
Member Author

@m1guelpf I added a new commit where we'll check first to see if the class exists before registering.

@m1guelpf
Copy link
Contributor

m1guelpf commented Jun 1, 2017

@driesvints What will happen to the providers on the config/app.php file?

@driesvints
Copy link
Member Author

@m1guelpf nothing. They'll continue to work as expected. That part remains untouched.

@Lloople
Copy link
Contributor

Lloople commented Jun 1, 2017

This looks amazing, I'm wondering why it's not been done before

@shirshak55
Copy link

Yes this is great idea. Even if a single package have multiple providers which usually don't happen can it work?
And I also would like after installing package it prompts for vendor publish like interface :)
I like this idea and hope it gets merged in Laravel 5.5

@driesvints
Copy link
Member Author

@bloggervista this indeed also works for multiple providers

@garygreen
Copy link
Contributor

garygreen commented Jun 1, 2017

Few problems I have with this:

  • What about certain packages that you would like to register based on a certain environment?
  • You can't change the order your own providers or packages are registered in - personally I think package should be registered first, so your own providers can overwrite anything if required, i.e. your own providers should have the final say.
  • If you want to overwrite a package's service provider, can this still be achieved? e.g. extends SomePackageServiceProvider and then only register that one?
  • What if packages have an extra field with providers in but it doesn't actually have anything to do with Laravel? It may break the whole process, just a consideration.
  • This looks like it doesn't work for deferred service providers.

Personally, I would prefer to see some kind of package:install command and some 'plugin' based system for Laravel where developers can better and more easily integrate/install their application more easily. I know there is stuff like ->loadMigrationsFrom and ->publishes blah but I think that's all a bit clunky as well, the whole plugin system needs a bit of tweaking.

@driesvints
Copy link
Member Author

@garygreen

What about certain packages that you would like to register based on a certain environment?

You can still turn off this behavior and use your own providers to have these checks. You can add a package to require-dev as well and not have it installed on production.

You can't change the order your own providers or packages are registered in - personally I think package should be registered first, so your own providers can overwrite anything if required, i.e. your own providers should have the final say.

You can turn off this behaviour through the config option and register the providers manually in app.php if you like just like before. Nothing changes to the current behavior.

What if packages have an extra field with providers in but it doesn't actually have anything to do with Laravel? It may break the whole process, just a consideration.

I thought of this and considered having an laravel_providers key instead but I'm not sure if we should do that. I think very few packages are using a key like that atm. It'll only break for very specific use cases. So I'd still go for just the providers key.

This looks like it doesn't work for deferred service providers.

This is true & we're checking in on that.

Personally, I would prefer to see some kind of package:install command and some 'plugin' based system for Laravel where developers can better and more easily integrate/install their application more easily. I know there is stuff like ->loadMigrationsFrom and ->publishes blah but I think that's all a bit clunky as well, the whole plugin system needs a bit of tweaking.

This is a different discussion I believe :)

@taylorotwell
Copy link
Member

@garygreen I have rewritten some things to address all of those concerns.

@garygreen
Copy link
Contributor

Nice, look forward to seeing it. It would be nice if the config:cache command gets rid of the packages manifest file and instead puts all the the providers in the app stuff - that way it won't need to bootstrap an extra class and additional file on every request in production.

@taylorotwell
Copy link
Member

It would be opcached in production anyways.

@antonkomarev
Copy link
Contributor

The only one thing about this way to loading service providers. What if I want to use package but extend service provider with my own one? Is it possible?

@driesvints
Copy link
Member Author

@a-komarev yes, there's a config option to turn off the behavior.

@driesvints
Copy link
Member Author

@a-komarev and you'll be able to not auto-discover individual packages.

@taylorotwell
Copy link
Member

All of the concerns expressed so far have been addressed. I am about to push the code.

@raftalks
Copy link
Contributor

raftalks commented Jun 16, 2017

This is nice feature, however the order of loading them might need further discussion, for instance I tried using laravel passport and it failed to the concept as it expects to have auth class defined in the container, where as that doesn't seem to exists.

@driesvints
Copy link
Member Author

@raftalks I think it's best that you open an issue on the passport repo for that

@shirshak55
Copy link

@driesvints What what order will the plugin will be auto loaded ? I mean is it like adding service provider at last ?

@driesvints
Copy link
Member Author

@bloggervista behavior was recently changed: #19646

@shirshak55
Copy link

Thanks for info i was worried about it :)

@vesper8
Copy link

vesper8 commented Aug 22, 2017

@driesvints what is the new config option to disable auto-discovery altogether ?

I am using the latest 5.5-dev and auto-discovery is enabled and working. But I can't seem to find any new option in any of the config files that have something to do with package discovery

@brunogaspar
Copy link
Contributor

@vesper8 Try defining 'autoload_package_providers' => false, on your config/app.php file and see if it works.

At least that's what's being used on this PR.

@driesvints
Copy link
Member Author

@vesper8 you can use this:

"extra": {
    "laravel": {
        "dont-discover": [
            "*"
        ]
    }
},

Here are the docs: https://laravel.com/docs/master/packages#package-discovery

@brunogaspar Note the quote in the first comment: the implementation here has been greatly refactored.

@vesper8
Copy link

vesper8 commented Sep 2, 2017

is there a way to list all the service providers and aliases that have been discovered ?

php artisan package:discover only lists the packages that contain service providers and/or aliases but since some packages can list multiple, or as I have now begun adding them myself for packages that are not adding it, it would be very useful to see what is actually being loaded

something like
php artisan package:discovered-aliases and php artisan package:discovered-providers
or php artisan package:discover --detailed

I know they are listed in /bootstrap/cache/packages.php but think it would be nice to have them listed in the console

edit: proposed my own PR to add this functionality #20938

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.