laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

Does ArtisanServiceProvider include too much services? #617

Open casperbakker opened 7 years ago

casperbakker commented 7 years ago

TLDR; Is it the right way to let ArtisanServiceProvider include lots of different commands that are dependent on other service providers? Makes it hard to excluded unneeded components from Laravel.

Details

We have a quite large SaaS app built in Laravel and we swapped out multiple components for our own implementation. For example the queueing system is totally our own. Naturally we would want to exclude the service providers that add the default queuing system.

But with Laravel 5.4 that is a bit weird, because the default ConsoleSupportServiceProvider includes ArtisanServiceProvider which registers a lot of commands. But all the command.queue.* commands do not work when the QueueServiceProvider is not registered. So in order to remove the QueueServiceProvider, we also need our own implementation of the ConsoleSupportServiceProvider and ArtisanServiceProvider to get rid of the artisan commands.

It feels like that is an oversight and unneeded coupling. Should the QueueServiceProvider not register their own artisan commands?

joshmanders commented 7 years ago

I was looking into this too because I wanted to override some default commands and found it strange that each component didn't register it's own commands in their providers.

I'd say submit a PR migrating them around and see what Taylor says.

skyrpex commented 7 years ago

I agree with this. Asked myself the same question a couple of times...

casperbakker commented 7 years ago

Glad others see it the same way 😄 But I question what the reason was behind this. Was it really just overlooked when making this change from Laravel 5.3 to 5.4?

I will let it sit here for a couple of days. If everyone agrees, I will make a PR with the right decoupling of Artisan commands.

sebastiandedeyne commented 7 years ago

I was looking into this too because I wanted to override some default commands and found it strange that each component didn't register it's own commands in their providers.

This would mean every package that has a CLI command would need to require Artisan though, which isn't ideal either.

tomschlick commented 7 years ago

This is a great idea.

This would mean every package that has a CLI command would need to require Artisan though, which isn't ideal either.

The package would already be requiring laravel/framework so it wouldn't need to require anything else... (unless I'm missing something here).

sebastiandedeyne commented 7 years ago

Illuminate components only require the bare necessity. If the CLI command would be part of Cache for example, illuminate/console would need to be added here.

https://github.com/illuminate/cache/blob/master/composer.json#L18-L19

tomschlick commented 7 years ago

Ah!, I was thinking of the third party packages not the internal framework ones. Got it.

joshmanders commented 7 years ago

Does it matter if it has the dependency in composer.json?

m1guelpf commented 7 years ago

@joshmanders @tomschlick Some projects use single illuminate components instead of the whole framework, so that would force them to download everything.