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

[5.3] Fire events when command starts and terminates #16355

Closed
wants to merge 1 commit into from
Closed

[5.3] Fire events when command starts and terminates #16355

wants to merge 1 commit into from

Conversation

bmitch
Copy link
Contributor

@bmitch bmitch commented Nov 11, 2016

No description provided.

@bmitch
Copy link
Contributor Author

bmitch commented Nov 11, 2016

Related to #15613

Thought I'd piggy back on the contributions by @themsaid and @lucasmichot to have events fired with a console command is starting and terminating.

One use case for this would be logging a message when a command starts and ends. I have scenarios where multiple commands are run throughout the night and this would be an easy way to have these messages logged.

@@ -102,6 +112,28 @@ public function call($command, array $parameters = [])
}

/**
* Runs the command container.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be just fine:

/**
 * {@inheritdoc}
 */

public function run(InputInterface $input = null, OutputInterface $output = null)
{
$this->events->fire(
new Events\CommandStarting($this->getCommandName($input), $input)
Copy link
Contributor

Choose a reason for hiding this comment

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

$this->getCommandName($input) is repeated twice and could be extracted to $commandName

* @param \Symfony\Component\Console\Input\InputInterface $input
* @return void
*/
public function __construct($command, InputInterface $input)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing $exitCode argument

@bmitch
Copy link
Contributor Author

bmitch commented Nov 11, 2016

Thanks for the feedback @lucasmichot I have updated the pull request.

{
$this->command = $command;
$this->input = $input;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

$exitCode should also be declared as a public property and set in the constructor.

@GrahamCampbell GrahamCampbell changed the title Fire events when command starts and terminates [5.3] Fire events when command starts and terminates Nov 12, 2016
@bmitch
Copy link
Contributor Author

bmitch commented Nov 12, 2016

Thanks again @lucasmichot

@taylorotwell
Copy link
Member

I don't know. I'm not very sold on this PR yet. If you have things you want to log from a command just put it in the command itself.

@Exadra37
Copy link

Exadra37 commented Nov 16, 2016

@taylorotwell

I see a very good use case here to avoid code duplication on very common task we use across hundreds of commands for mission critical batch processes that run all the day, some of them every 5 minutes.

On Command Start:

  • $nagios->start();
  • $log->info("{$startTime}|{$uniqueIdentifier}: Command {$commandName} starting");

On Command End:

  • $nagios->success()
  • $log->info("{$endTime}|{$uniqueIdentifier}: Command {$commandName} ending");

Advantages:

  • So with this pull request we will avoid to have to declare this piece of logic in hundreds of command we need to maintain.
  • In case we need to fix or change how we log and monitor commands start and end we only need to do it in 1 place, not in hundreds of places.
  • This will allow more maintainable and clean code.

Please consider to merge it again.

Why Log and Monitor:

  • We have automated tools parsing ElasticSearch logs to check for commands execution time and creating alerts when they exceed a threshold.
  • Plus we have the Nagios checks alerting us when some Command fails to terminate.

@JosephSilber
Copy link
Member

JosephSilber commented Nov 16, 2016 via email

@Exadra37
Copy link

We use micro services architecture and Inheritance is not used in our code base...

And yes it can be achieved in several different ways, but if the Framework can easily handle that with events, like in the Models, than no reason to duplicate efforts across several packages or create dependencies to handle this bit of logic.

@lucasmichot
Copy link
Contributor

Can this not be reconsidered @taylorotwell ?

@bmitch
Copy link
Contributor Author

bmitch commented Nov 23, 2016

For those interested I ended up creating a package that should fill the need:

https://github.com/bmitch/consoleEvents

Any feedback is greatly appreciated. Thanks.

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.

5 participants