8000 [Console] Allow a command iterator to be added to console app by andytson · Pull Request #24990 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 1 commit into from
Closed

[Console] Allow a command iterator to be added to console app #24990

wants to merge 1 commit into from

Conversation

andytson
Copy link
@andytson andytson commented Nov 16, 2017
Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR

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.

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.
@andytson
Copy link
Author

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.

@ro0NL
Copy link
Contributor
ro0NL commented Nov 16, 2017

I suggested to expand to array if typed against array. Would fit here no?

Something along the lines of #22204 (#22204 (comment)).

I guess !tagged truly implies "iterable". Not just iterator, nor array. Hence i think it makes sense to apply iterator_to_array if needed.

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)
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Member
@chalasr chalasr Nov 16, 2017

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.

@andytson
Copy link
Author

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

@andytson andytson closed this Nov 16, 2017
@andytson andytson deleted the patch-1 branch November 16, 2017 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0