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

[Console][Messenger] add RunCommandMessage and RunCommandMessageHandler #49814

Merged
merged 1 commit into from Jul 30, 2023

Conversation

kbond
Copy link
Member

@kbond kbond commented Mar 25, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR todo

Similar to #49813, when using the scheduler it could be useful to execute commands.

Usage

use Symfony\Component\Console\Exception\RunCommandFailedException;
use Symfony\Component\Console\Messenger\RunCommandMessage;

try {
    $context = $bus->dispatch(new RunCommandMessage('my:command'));

    $context->output; // string - output of command
    $context->exitCode; // int - the exit code
catch(RunCommandFailedException $e) { // if exit code is non-zero or command threw exception
    $e->context->output; // string - output of command
    $e->context->exitCode; // int - the exit code
    $e->getPrevious(); // null|\Throwable exception command threw if applicable
}

// "never" fail
$context = $bus->dispatch(new RunCommandMessage('my:command', throwOnNonSuccess: false, catchExceptions: true));

TODO:

  • wire up
  • tests

@kbond kbond requested a review from chalasr as a code owner March 25, 2023 17:58
@carsonbot carsonbot added Status: Needs Review Console Feature Messenger RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Mar 25, 2023
@carsonbot carsonbot added this to the 6.3 milestone Mar 25, 2023
@carsonbot carsonbot changed the title [Console][Messenger][RFC] add ExecuteCommand and ExecuteCommandHandler [Console][Messenger] add ExecuteCommand and ExecuteCommandHandler Mar 25, 2023
@kbond
Copy link
Member Author

kbond commented Mar 25, 2023

Like #49813 (comment), maybe CommandMessage and CommandMessageHandler would be better names?

@kbond kbond removed the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Mar 27, 2023
$input = new StringInput($message->input);
$output = new BufferedOutput();

$this->application->setCatchExceptions($message->catchExceptions);
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I clone the application here instead of having the service not-shared?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe try/finally to modify state on the shared service?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I can do this as there's no way to access the current state of Application::$catchExceptions.

@kbond
Copy link
Member Author

kbond commented Mar 28, 2023

One thing I think is missing: making the output available if there's an exception. Consider a system that tracks messages and emails an admin on failure. I can attest that this is valuable information.

What about wrapping the caught exception in a new CommandFailedException that contains the output?

In #49813 this is possible because the thrown exception (ProcessFailedException) includes the Process instance which makes the output available.

) {
}

public function __toString(): string
Copy link
Contributor

Choose a reason for hiding this comment

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

see also #49813 (comment)

src/Symfony/Component/Console/Messenger/ExecuteCommand.php Outdated Show resolved Hide resolved
$input = new StringInput($message->input);
$output = new BufferedOutput();

$this->application->setCatchExceptions($message->catchExceptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe try/finally to modify state on the shared service?

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@kbond
Copy link
Member Author

kbond commented Jul 28, 2023

I've updated this PR:

  1. Renamed the message to RunCommandMessage.
  2. Added RunCommandContext to be returned from the handler
  3. Added RunCommandFailedException that's thrown on failure that wraps the original exception and contains RunCommandContext
  4. Configuration for throwing RunCommandFailedException on non-successful exit codes

(updated PR description to show usage)

@kbond kbond changed the title [Console][Messenger] add ExecuteCommand and ExecuteCommandHandler [Console][Messenger] add RunCommandMessage and RunCommandMessageHandler Jul 28, 2023
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM, @kbond I've made a rename suggestion.

@fabpot
Copy link
Member

fabpot commented Jul 30, 2023

Thank you @kbond.

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.

None yet

5 participants