[Console] Fix registering lazy command services with autoconfigure enabled #23556
Conversation
chalasr
force-pushed
the
console-command-tag
branch
from
e29306b to
b428154
Compare
|
Now if you have something like the following, the second tags:
- { name: console.command, command: foo }
- { name: console.command, command: bar, alias: foobar } |
chalasr
force-pushed
the
console-command-tag
branch
from
b428154 to
3d1dcf7
Compare
|
Right, it's worse. The command must have only one command name and may have aliases, we need a rule that doesn't feel magic for determining it. Updated this, now your example would throw:
The first |
| ->setPublic(false) | ||
| ->addTag('console.command', array('command' => 'my:command')) | ||
| ->addTag('console.command', array('command' => 'other:name')) | ||
| ; |
There was a problem hiding this comment.
Still think this is the best option actually :)
specially because relying on the order on which tags are defined sounds less good to me
I guess it aims for to be explicit what getName returns, but do we care? It's the first one :) and practically there's no difference either. Low level perhaps, but thats not an issue for end users.
Alternatively; command: name, aliases: [name, nameN] and dont allow console.command more than once.
Alter-alternatively; perhaps command is a bad name due term duplication with console.command, what about only name: x?
There was a problem hiding this comment.
Alternatively;
command: name, aliases: [name, nameN]
Tag attributes cannot be arrays :) that's what I'm trying to mimic
perhaps command is a bad name due term duplication with console.command, what about only name?
Not available :) - { name: console.command, command: foobar }
The command attribute is fine to me. What I don't like much is to call setName() on the command (mandatory) for the first command attribute found, and consider other as aliases. But if others think it's better, I'm ok for changing it
There was a problem hiding this comment.
Tag attributes cannot be arrays
Didnt know that :| And for name well.. eh, right. Lets pretend that did not happen :)
There was a problem hiding this comment.
What I don't like much is to call setName() on the command (mandatory) for the first command attribute found, and consider other as aliases.
Cant/shouldnt you do that from CommandLoader::get actually?
|
I tend to agree now that getting rid of the |
chalasr
force-pushed
the
console-command-tag
branch
from
3d1dcf7 to
167f191
Compare
|
Updated for @Tobion suggestion. |
chalasr
force-pushed
the
console-command-tag
branch
from
167f191 to
8a71aa3
Compare
|
Thank you @chalasr. |
…oconfigure enabled (chalasr)
This PR was merged into the 3.4 branch.
Discussion
----------
[Console] Fix registering lazy command services with autoconfigure enabled
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | n/a
| License | MIT
| Doc PR | n/a
For
```yaml
_defaults:
autoconfigure: true
App\:
resource: '../../src/*'
App\Command\FooCommand:
tags:
- { name: console.command, command: foo }
```
Before you get the following error:
> Missing "command" attribute on tag "console.command" for service "App\Command\FooCommand"
Now the command is lazy.
----
Btw, @Tobion's #22734 (comment)
> Wouldn't it be more straightforward if aliases are just the additional tags using the command attribute as well?
Then there is no need for an alias property at all and this strange condition doesn't apply either.
Partially addressed here by removing the need for repeating the `command` attribute on each `console.command` tag
```yaml
# before
tags:
- { name: console.command, command: foo }
- { name: console.command, command: foo, alias: foobar }
# after
tags:
- { name: console.command, command: foo }
- { name: console.command, alias: foobar }
```
Tobias proposal:
```yaml
tags:
- { name: console.command, command: app:my-command }
- { name: console.command, command: app:my-alias }
```
I wanted to propose exactly the same at first, but finally found more clear to add a specific attribute for aliases, especially because relying on the order on which tags are defined sounds less good to me. Please tell me about your preference.
(And sorry for the noise around this feature, I want to polish it for 3.4)
Commits
-------
8a71aa3 Fix registering lazy command services with autoconfigure enabled
7 participants
For
Before you get the following error:
Now the command is lazy.
Btw, @Tobion's #22734 (comment)
Partially addressed here by removing the need for repeating the
commandattribute on eachconsole.commandtagTobias proposal:
I wanted to propose exactly the same at first, but finally found more clear to add a specific attribute for aliases, especially because relying on the order on which tags are defined sounds less good to me. Please tell me about your preference.
(And sorry for the noise around this feature, I want to polish it for 3.4)