10000 [Console][Messenger] add `RunCommandMessage` and `RunCommandMessageHandler` by kbond · Pull Request #49814 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

8000 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

Jump to bottom
Merged
merged 1 commit into from
Jul 30, 2023
Merged

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

merged 1 commit into from
Jul 30, 2023

Conversation

kbond
Copy link
Member
@kbond kbond commented Mar 25, 2023 •
edited
Loading
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 force-pushed the messenger-command branch from 65c65a0 to 592dc48 Compare March 27, 2023 15:27
@kbond kbond removed the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Mar 27, 2023
kbond
kbond commented Mar 27, 2023
View reviewed changes
src/Symfony/Component/Console/Messenger/ExecuteCommandHandler.php Outdated
$input = new StringInput($message->input);
$output = new BufferedOutput();

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

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 kbond force-pushed the messenger-command branch 2 times, most recently from 848bdb7 to f712569 Compare March 27, 2023 15:56
@kbond kbond mentioned this pull request Mar 27, 2023
Merged
2 tasks
@kbond
Copy link
Member Author
kbond commented Mar 28, 2023 •
edited
Loading

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.

ro0NL
ro0NL reviewed Apr 25, 2023
View reviewed changes
src/Symfony/Component/Console/Messenger/ExecuteCommand.php Outdated
) {
}

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/ExecuteCommandHandler.php Outdated
$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 kbond force-pushed the messenger-command branch from f712569 to 0cd0b54 Compare July 28, 2023 13:19
@kbond
Copy link
Member Author
kbond commented Jul 28, 2023 •
edited
Loading

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
@kbond kbond mentioned this pull request Jul 29, 2023
Closed
fabpot
fabpot approved these changes Jul 30, 2023
View reviewed changes
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.

@kbond kbond force-pushed the messenger-command branch from 0cd0b54 to 10f89d1 Compare July 30, 2023 13:22
@fabpot fabpot force-pushed the messenger-command branch from 10f89d1 to dd5b0b7 Compare July 30, 2023 13:24
@fabpot
Copy link
Member
fabpot commented Jul 30, 2023

Thank you @kbond.

@fabpot fabpot merged commit fe30946 into symfony:6.4 Jul 30, 2023
@fabpot fabpot mentioned this pull request Jul 30, 2023
Closed
This was referenced Oct 21, 2023
Merged
Merged
@kbond kbond mentioned this pull request Oct 24, 2023
Closed
@xabbuh xabbuh mentioned this pull request Nov 2, 2023
Merged
@kbond kbond deleted the messenger-command branch November 14, 2024 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ro0NL ro0NL ro0NL left review comments

@fabpot fabpot fabpot approved these changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Console Feature Messenger Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
5 participants
@kbond @fabpot @ro0NL @nicolas-grekas @carsonbot
0