10000 [Scheduler] Add MessageHandler result to the `PostRunEvent` by bartholdbos · Pull Request #58546 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Scheduler] Add MessageHandler result to the PostRunEvent #58546

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

By clicking “Sign up for GitHub”, you 8000 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
Feb 7, 2025
Merged

[Scheduler] Add MessageHandler result to the PostRunEvent #58546

merged 1 commit into from
Feb 7, 2025

Conversation

bartholdbos
Copy link
Contributor
@bartholdbos bartholdbos commented Oct 11, 2024 •
edited by fabpot
Loading
Q A
Branch? 7.3
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 bartholdbos requested a review from kbond as a code owner October 11, 2024 18:31
@carsonbot carsonbot added this to the 7.2 milestone Oct 11, 2024
@slutsky
Copy link
slutsky commented Oct 11, 2024 •
edited
Loading

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.

@bartholdbos
Copy link
Contributor Author

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 bartholdbos force-pushed the scheduler-post-run-event-result branch from 1705445 to 4e56ad4 Compare October 18, 2024 09:46
@bartholdbos
Copy link
Contributor Author

@kbond Could you take a look at this maybe?

kbond
kbond reviewed Oct 25, 2024
View reviewed changes
Copy link
Member
@kbond kbond left a comment

Choose a reason for hiding this comment

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

I think this makes sense. @alli83, have any thoughts?

@bartholdbos bartholdbos force-pushed the scheduler-post-run-event-result branch from 03dceb0 to 468c02d Compare October 25, 2024 13:34
@bartholdbos
Copy link
Contributor Author
bartholdbos commented Oct 25, 2024 •
edited
Loading

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 bartholdbos force-pushed the scheduler-post-run-event-result branch from 468c02d to 959b270 Compare October 26, 2024 10:04
@bartholdbos
Copy link
Contributor Author

@alli83 or @kbond could you review this maybe?

@alli83
Copy link
Contributor
alli83 commented Oct 29, 2024 •
edited
Loading

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.

kbond
kbond requested changes Oct 31, 2024
View reviewed changes
Copy link
Member
@kbond kbond left a comment

Choose a reason for hiding this comment

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

I'm good with this feature. Can you add an entry to the changelog?

kbond
kbond approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member
@kbond kbond left a comment

Choose a reason for hiding this comment

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

7.2 is in feature freeze so this will likely not make it until 7.3.

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@bartholdbos bartholdbos force-pushed the scheduler-post-run-event-result branch from 268e589 to 05fe4a3 Compare December 13, 2024 10:02
@bartholdbos
Copy link
Contributor Author

@kbond I have rebased the branch on 7.3

kbond
kbond approved these changes Dec 13, 2024
View reviewed changes
8000
Copy link
Member
@kbond kbond left a comment

Choose a reason for hiding this comment

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

Looks good!

@bartholdbos, would you like to try updating the test? I was thinking here, instead of setting the props to true, set them to the event object and then assert on them in the test. If not, no worries, I can add after merge.

@bartholdbos
Copy link
Contributor Author

@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?

kbond
kbond requested changes Dec 20, 2024
View reviewed changes
Copy link
Member
@kbond kbond left a comment

Choose a reason for hiding this comment

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

Yes, that looks great!

@bartholdbos bartholdbos requested a review from kbond December 20, 2024 16:27
@bartholdbos
Copy link
Contributor Author

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?

@kbond
Copy link
Member
kbond commented Dec 20, 2024

Just need to wait for another core team member approval.

@bartholdbos
Copy link
Contributor Author

@fabpot Could you maybe take a look at this?

@OskarStark OskarStark changed the title [Scheduler] Add MessageHandler result to the PostRunEvent [Scheduler] Add MessageHandler result to the PostRunEvent Jan 30, 2025
@kbond
Copy link
Member
kbond commented Feb 5, 2025

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

@fabpot fabpot force-pushed the scheduler-post-run-event-result branch from 3e9c00b to 306eeb6 Compare February 7, 2025 08:23
@fabpot
Copy link
Member
fabpot commented Feb 7, 2025

Thank you @bartholdbos.

@fabpot fabpot merged commit 201747b into symfony:7.3 Feb 7, 2025
8 of 9 checks passed
@fabpot fabpot mentioned this pull request Feb 7, 2025
Closed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
< 64A0 div class="discussion-sidebar-heading text-bold"> Reviewers

@fabpot fabpot fabpot approved these changes

@kbond kbond kbond approved these changes

Assignees
No one assigned
Labels
Feature Scheduler Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
6 participants
@bartholdbos @slutsky @alli83 @kbond @fabpot @carsonbot
0