symfony icon indicating copy to clipboard operation
symfony copied to clipboard

[Scheduler] Add MessageHandler result to the PostRunEvent

Open bartholdbos opened this issue 1 year ago • 1 comments

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

This PR will add the result of a MessageHandler to the PostRunEvent of the Scheduler Component. This can be used for example for retrieving the output of a RunCommandMessage scheduled by the scheduler component.

<?php

namespace App\Service;

use Symfony\Component\Console\Messenger\RunCommandMessage;
use Symfony\Component\Scheduler\Attribute\AsSchedule;
use Symfony\Component\Scheduler\RecurringMessage;
use Symfony\Component\Scheduler\Schedule;
use Symfony\Component\Scheduler\ScheduleProviderInterface;

#[AsSchedule]
class ScheduleProvider implements ScheduleProviderInterface
{
    public function getSchedule(): Schedule
    {
        $schedule = new Schedule();
        $schedule->add(RecurringMessage::cron('* * * * *', new RunCommandMessage('about')));

        return $schedule;
    }
}
<?php

namespace App\EventListener;

use Symfony\Component\Console\Messenger\RunCommandContext;
use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
use Symfony\Component\Scheduler\Event\PostRunEvent;

#[AsEventListener(event: PostRunEvent::class, method: 'onMessage')]
class SchedulerListener
{
    public function onMessage(PostRunEvent $event): void
    {
        $result = $event->getResult();

        if ($result instanceof RunCommandContext) {
            echo $result->output; //Log or store output somewhere
        }
    }
}

bartholdbos avatar Oct 11 '24 18:10 bartholdbos

How does it work if the handler result type is "never"?

I think It's better to use a message to transfer result. Possible to use a setter to add a result to the message inside a handler.

slutsky avatar Oct 11 '24 20:10 slutsky

How does it work if the handler result type is "never"?

I think It's better to use a message to transfer result. Possible to use a setter to add a result to the message inside a handler.

If the MessageHandler does never return the $result value will be null. If the messagehandler is not in your own control(for instance RunCommandMessageHandler) it is not possible to use a setter to set the result to the message from the handler. The HandledStamp already collects the result of the message handler. This change will only allow you to use this result in the scheduler component as well.

bartholdbos avatar Oct 18 '24 09:10 bartholdbos

@kbond Could you take a look at this maybe?

bartholdbos avatar Oct 25 '24 10:10 bartholdbos

Thanks for the reply. I have also changed the ServiceCallMessageHandler to return it's result. This way the PostRunEvent result would also be available for AsPeriodicTask and AsCronTask. Also when using RedispatchMessage with a sync:// transport configured while developing I noticed the result didn't come through so I also returned the result in the RedispatchMessageHandler. Let me know what you think of these latest changes.

bartholdbos avatar Oct 25 '24 13:10 bartholdbos

@alli83 or @kbond could you review this maybe?

bartholdbos avatar Oct 29 '24 17:10 bartholdbos

Retrieving the result could indeed be interesting e.g depending on the outcome, we might decide whether to continue or stop. However, regarding the logging example, imho, I wonder if it’s really necessary to handle it in the listener rather than directly within the handler.

alli83 avatar Oct 29 '24 21:10 alli83

@kbond I have rebased the branch on 7.3

bartholdbos avatar Dec 13 '24 10:12 bartholdbos

@kbond Sure, I have updated the test to use the event object instead of the booleans. It can now assert on properties that are in the event, is this what you meant?

bartholdbos avatar Dec 20 '24 15:12 bartholdbos

Thanks for the review @kbond! I think it's ready on my side, is another review required or is it ready for merging by you/somebody else?

bartholdbos avatar Dec 20 '24 17:12 bartholdbos

Just need to wait for another core team member approval.

kbond avatar Dec 20 '24 17:12 kbond

@fabpot Could you maybe take a look at this?

bartholdbos avatar Jan 30 '25 15:01 bartholdbos

@bartholdbos, since this has been approved, it'll likely get approved by another core member and merged closer to the 7.3 feature freeze.

kbond avatar Feb 05 '25 12:02 kbond

Thank you @bartholdbos.

fabpot avatar Feb 07 '25 08:02 fabpot