Auto-Registration of Commands is now configurable #6389
Conversation
|
What's the point? If you register a bundle, why would you want to not register the commands? When does it hurt? |
|
@fabpot see the email @seiffert send to the list, the idea is to make it possible for Bundle's to register commands as services instead. those who want to unit test their commands as well as maintain a better overview of their dependencies might prefer them to be services. furthermore it would mean i would no longer need to do hacks like this to conditionally register commands https://github.com/doctrine/DoctrinePHPCRBundle/blob/master/DoctrinePHPCRBundle.php#L48 |
|
BTW, it appears we should add an interface for the entire command handling as |
|
@fabpot The point is that currently the only "right" way to add commands to an application is to put them into a specific directory (which should be a convention). See console-extra-bundle for an example that registers commands in another way. There, I am using an DI tag to determine which services should be considered console commands. |
|
👎 it is just as fast to overwrite the registerCommands function as it is with writing the auto register one. |
|
Also by doing |
|
@henrikbjorn The problem with overriding registerCommands is that it would break the Bundle's API. (When calling registerCommands, you expect that commands are being registered...) Where am I calling |
|
in your bundle you are. It would break the bundle's api? How. It still does the same thing registering commands, it is just not autodiscovering them. Saying that would break the API then every extension of a class would be an API breakage. |
|
The bundle console-extra-bundle does not register own commands. The method SeiffertConsoleExtraBundle::registerCommands() is used to register all commands collected during container compilation. I am calling A true extension of a class does not break any API. However, if you take away functionality, a class cannot fullfil promises made by the parent class. (In this case, you are breaking API and should consider re-designing your class) |
|
Then the describtion of the registerCommand method should be changed. To only saying "Register commands with the Console Application" instead of the finding stuff. |
|
BTW I was also pondering if the auto registration shouldn't be disabled via an interface. them again its weird to disable a capability via an interface. alternatively we could offer 2 Bundle base classes one with and without the interface for registerCommand |
|
@henrikbjorn Yes, that would be a good alternative. The bundle would then be responsible for registering it's commands in all cases and we could move auto-discovery into a separate method like |
|
@lsmith77 that is just adding bloat, change the description and implement an empty registerCommands if you dont want the autodiscovering i think that is the best approach. |
|
@henrikbjorn You would still need some kind of a switch... |
|
I wouldnt need a switch just dont call parent::registerCommands. |
|
are you refering to this |
|
And instead, you want them to override another method (with a non-empty implementation) added only to avoid the overriding... This is a weird reasonning IMO. |
|
No, I want the user to be able to decide whether commands are being auto-discovered or not. I want to give him a way to configure this other than removing functionality by overriding a method with an empty implementation. I am open for alternatives, but the current "solution" is not a real option for me. |
|
Why is it not a real option ? your solution also requires to overwrite a method (and it requires to ship an implementation in the overwritten method when you actually want your bundle to do nothing) |
|
This is not about saving bytes but about extending functionality in a clean way. Let's say I created a bundle X which overrides The example might be abstruse, but it illustrates the problem I have with empty implementations. |
|
indeed, its just bad design to have to overwrite this method this way. so i agree it would be better to either have a flag or an interface to express that you want or don't want the auto registration feature. additionally we also need to include |
|
Probably @lsmith77's idea with the additional interface is better. If a bundle class implements this interface, it has to provide a @lsmith77 What would be a good name/place for such an interface? I see two possible semantics for it: Either, it inherits from |
|
OK, after re-reading the code, I suggest adding an interface |
|
the name sounds fine to me .. but for backwards compatibility i fear we will have to have the |
|
Although #8182 looks a little hacky to me, it probably solves the issue for most cases. |
This PR adds a method to HttpKernel\Bundle that is used to decide whether the commands of the bundle should be registered using Bundle::registerCommands.
This needs to be configurable since there are cases in which commands should not be registered by default. One could just override Bundle::registerCommands to solve this. However, this wouldn't be compatible with LSP.