Skip to content
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

Fixes #65: Allow commands to call other commands #65

Closed

Conversation

grasmash
Copy link
Contributor

No description provided.

@grasmash grasmash changed the title Stubbing out new @calls annotation. Fixes #65: Allow commands to call other commands Jan 17, 2017
@grasmash
Copy link
Contributor Author

This introduces the new @calls tag, but does not actually do anything with it yet.

@grasmash grasmash force-pushed the issue-64-call-commands branch from 4ebc3f8 to 2647005 Compare January 17, 2017 19:18
if (is_string($calls)) {
$calls = explode(',', static::convertListToCommaSeparated($calls));
}
$this->calls = array_filter($calls);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe array-merge these together, so folks can use multiple @calls in addition to csv, if they prefer?

Copy link
Contributor Author

@grasmash grasmash Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'd prefer that. I briefly attempted this but wasn't quite sure how to access multiple @calls values, each usage appeared to overwrite the previous.

protected function executeCallsCommands() {
if ($this->getCalls()) {
foreach ($this->getCalls as $command_name) {
// @todo validate that $command_name exists in this application.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the interesting part. The $application field of Symfony Console's command class is private. 😢

We could override setApplication to cache our own copy of it. Not super great.

$command = $this->getApplication()->find($command_name);
if (!$command) {
/** @var LoggerInterface $logger */
//$logger->warning("Command $command_name does not exist. Skipping.");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a logging mechanism available?

@grasmash
Copy link
Contributor Author

@greg-1-anderson Let's make a list of what it will take to merge this in.

  • Allow multiple @Calls annotations to be used
  • Allow argument values to be passed via @Calls annotations
  • Move executeCallsCommands() logic to ... somewhere else. @greg-1-anderson You briefly mentioned a specific idea for how to better implement this, can you elaborate?

*/
protected function processCalls($tag)
{
$this->commandInfo->setCalls((string)$tag->getDescription());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are multiple @Calls annotations, this function will be called multiple times. Currently, this implementation overwrites via setCalls(). It could also call getCalls() first and merge. Might want to move where the csv is converted to an array here, maybe.

@greg-1-anderson
Copy link
Member

I have reservations about Allow argument values to be passed via @calls annotations. Once we are calling other commands with annotations, then we need parameters, then we need to process substitutions on the parameters, then maybe we need to call a function to get the substitution values. Annotations shouldn't become their own way to write code.

I tend to think that this implementation should be abandoned, and we should make it easier for one command to call another via PHP.

@grasmash
Copy link
Contributor Author

grasmash commented Feb 1, 2017

@greg-1-anderson I'm ok with abandoning this implementation and following up with a different plan in #64.

I mostly want to avoid:

<?php

  /**
   * @command tests:all
   */
  public function testsAll() {
    $result = $invoker->invoke('tests:behat');
    if (!$result) {
      return 1;
    }
    $result = $invoker->invoke('tests:phpunit');
    if (!$result) {
      return 1;
    }
    $result = $invoker->invoke('tests:custom');
    if (!$result) {
      return 1;
    }
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants