8000 [RFC] Add static method + interface to get command names at compile time · Issue #23796 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC] Add static method + interface to get command names at compile time #23796

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.

Sign up for GitHub

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

Jump to bottom
Closed
nicolas-grekas opened this issue Aug 5, 2017 · 8 comments
Closed

[RFC] Add static method + interface to get command names at compile time #23796

nicolas-grekas opened this issue Aug 5, 2017 · 8 comments
Assignees
@chalasr
Labels
Console RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Milestone
3.4

Comments

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 5, 2017 •
edited
Loading
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 3.4

With PSR-4-based service definitions, we can declare command as services. But this doesn't provide laziness for them, because we don't know their name at compile time.

To enhance that, what about adding a new static method + related interface to commands, ?
Eg.

public static function getDefaultName()
{
    return 'foo:bar';
}

That would also enable auto-configuration of commands. The method would be called to fill in the "name" attribute of the "console.command" tag when none is set.

ping @chalasr
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 5, 2017
@javiereguiluz
Copy link
Member

Instead of just defining the name, we could define the entire command signature (like Laravel does). This would save us all the verbosity required to define a command.

@nicolas-grekas
Copy link
Member Author

@javiereguiluz, creating a DSL for command configuration is something else, that's for another RFC :)

@javiereguiluz javiereguiluz added Console RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Aug 5, 2017
@ro0NL
Copy link
Contributor
ro0NL commented Aug 6, 2017

why the new interface? we can simply add this to Command no?

@chalasr
Copy link
Member
chalasr commented Aug 6, 2017

This has been suggested by @weaverryan and briefly discussed in #22734.
from #22734 (comment):

@chalasr please don't make things static. This is even worse for configurability, as you cannot make the name configurable anymore in commands (while it is currently), except by using some global static state (which is very bad), and only if you don't register the same command class multiple times with different names (and different instances of your deps, which is the use case for registering multiple times)
[...]
Thus, it will be almost impossible to write a good migration path for this: you cannot make the default implementation of the static method call the existing code, as static cannot call non-static code; so it means you still have to allow commands not implementing this static-based interface everywhere in the code.

The idea is not to replace the tag nor to make Application aware of this method but just to fill the tag attribute by calling it from the pass.
Commands relying on this interface could not be used for more than 1 service. Given this is all about auto discovered/configured commands, I think it's not an issue. Explicit configuration would always win so that If one wants to reuse a command for another service, then one has to fill the tag attribute for this service.

👍 as it makes laziness accessible to autoconfigured commands.

@ro0NL
Copy link
Contributor
ro0NL commented Aug 6, 2017

Explicit configuration would always win

But getName doesnt always win from getDefaultName, which may be confusing. I think that's a documentation issue =/

Commands relying on this interface

:S wouldnt getName() be something like return $this->name ?? static::getDefaultName()?

@nicolas-grekas
Copy link
Member Author

@chalasr thanks for the link. There is no pb to me to make this static method if it is really an easilly overrideable default, which is what I'm suggesting to do. You can think of it like an annotation that is parseable by PHP itself.

@chalasr
Copy link
Member
chalasr commented Aug 9, 2017

Going to pick this one.
Not sure I can expect anything but... any hint on how could this interface be named would be nice :)

@nicolas-grekas
Copy link
Member Author

DefaultNameProviderInterface

@chalasr chalasr self-assigned this Aug 10, 2017
@ro0NL ro0NL mentioned this issue Aug 10, 2017
Merged
@chalasr chalasr mentioned this issue Aug 14, 2017
Merged
fabpot added a commit that referenced this issue Aug 24, 2017
@fabpot
feature #23887 [Console] Allow commands to provide a default name for…
67abb80 
… compile time registration (chalasr, nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Allow commands to provide a default name for compile time registration

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23796
| License       | MIT
| Doc PR        | symfony/symfony-docs#8147

Commits
-------

eda7d42 [Console] Add protected static $defaultName to set the default name of a Command
5d9ae6b [Console] Allow commands to provide a default name for compile time registration
@chalasr chalasr closed this as completed Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chalasr chalasr

Labels
Console RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
3.4
Development

No branches or pull requests
4 participants
@javiereguiluz @nicolas-grekas @ro0NL @chalasr
0