-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] Allow a command iterator to be added to console app #24990
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
Conversation
Symfony 3.4/4.0 brings in the ability to use !tagged in service definitions to pass tagged services as a argument. It uses a RewindableGenerator class, so the typehint here doesn't accept it even though it just iterates over the commands. This change will allow a custom console app to be created with a service method call in the definition. The typehint is only available in PHP 7.1+ though, so isn't compatible with Symfony 3.4's targeted PHP versions, but is for 4.0.
Having reminded myself of the new lazy command functionality, I think I'll go with using that's command loader for the use I was doing for custom console apps, as this !tagged way wouldn't support lazy commands. I suppose it'd be interesting then if there'd be a way to support lazy objects in other ways without having to write compiler passes. |
I suggested to expand to array if typed against array. Would fit here no? Something along the lines of #22204 (#22204 (comment)). I guess cc @nicolas-grekas we forgot this basically ;-) WDYT? The alternative is of course to change method sigs on a per case base. Not sure we want that. |
*/ | ||
public function addCommands(array $commands) | ||
public function addCommands(iterable $commands) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a BC break unfortunately, as it's a public method, so we can't change this prototype before 5.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a BC break when PHP 7.1 is required in Symfony 4, which is still beta. It's a super class/primative type hint, which accepts arrays as well as iterators. It only allows more than before, however though in the event this method is overriden in a sub-class, then it'd not satisfy the function signature, if that counts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Application API expects $commands
to be an array elsewhere, passing an iterable here would break at e.g. find()
. So we would need to iterator_to_array()
. Is there a real benefit? I don't see one personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we iterate over the result just below, nevermind.
closing as beta's don't allow non-BC changes and waiting for a 5.0 for a change I don't actually need now doesn't seem worthwhile unless done across more of the framework |
Symfony 3.4/4.0 brings in the ability to use !tagged in service definitions to pass tagged services as a argument. It uses a RewindableGenerator class, so the typehint here doesn't accept it even though it just iterates over the commands.
This change will allow a custom console app to be created with a service method call in the definition.
The typehint is only available in PHP 7.1+ though, so isn't compatible with Symfony 3.4's targeted PHP versions, but is for 4.0.