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

[Scheduler] Separate id and description in message providers #52874

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

valtzu
Copy link
Contributor

@valtzu valtzu commented Dec 3, 2023

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52853
License MIT

Separate id and description in message providers to keep debug:schedule output clean while allowing arbitrary distinct id.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "6.4".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@valtzu
Copy link
Contributor Author

valtzu commented Dec 3, 2023

Not sure what to do with the tests since FrameworkBundle tests now depend on the unreleased/unmerged Scheduler version (this PR).

@valtzu valtzu force-pushed the 52853-scheduler-arguments-fix branch from d5f9d45 to 52423ea Compare December 6, 2023 10:43
@alex-dev
Copy link
Contributor

alex-dev commented Dec 6, 2023

While you're addressing this, could you check other __toString scheduled messages? RedispatchMessage is also affected.

@valtzu
Copy link
Contributor Author

valtzu commented Dec 6, 2023

While you're addressing this, could you check other __toString scheduled messages? RedispatchMessage is also affected.

@alex-dev but RedispatchMessage uses the wrapped message's __toString() and it does include the transport names in __toString so I don't think it's affected itself directly?


Or yeah if you don't implement __toString in your message, then it is affected indeed. But since right now __toString is how you define uniqueness, I would consider that a feature and not a bug.

@alex-dev
Copy link
Contributor

alex-dev commented Dec 6, 2023

You don't define uniqueness only by __toString. If a message doesn't implement a __toString, Schedule use serialize. So, if a message wrapped by a RedispatchMessage is not Stringable, they should be serialized to determine uniqueness. RedispatchMessage should be transparent for identifying uniqueness.

@valtzu valtzu force-pushed the 52853-scheduler-arguments-fix branch 2 times, most recently from 687a3d6 to bd6144e Compare December 6, 2023 19:30
@valtzu
Copy link
Contributor Author

valtzu commented Dec 6, 2023

Ok then, I separated id & description and used hashed serialized message for uniqueness with StaticMessageProvider.

I would've rather added explicit getDescription() on MessageProviderInterface but with optional __toString we should be able to avoid dealing with BC.

What do you think?

We could also consider adding id parameter to #[AsPeriodicTask] & #[AsCronTask] attributes in case someone would rather have static ids which don't depend on message content.

@valtzu valtzu changed the title [Scheduler] Distinguish ServiceCallMessage arguments in scheduler [Scheduler] Separate id and description in message providers Dec 6, 2023
@alex-dev
Copy link
Contributor

alex-dev commented Dec 6, 2023

Seems good!

@valtzu valtzu requested a review from fabpot December 16, 2023 14:12
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM with a minor comment

src/Symfony/Component/Scheduler/RecurringMessage.php Outdated Show resolved Hide resolved
src/Symfony/Component/Scheduler/RecurringMessage.php Outdated Show resolved Hide resolved
@nicolas-grekas
Copy link
Member

Thank you @valtzu.

@nicolas-grekas nicolas-grekas merged commit a84d42b into symfony:6.4 Jan 2, 2024
1 of 3 checks passed

return new self($trigger, new StaticMessageProvider([$message], $description));
return new self($trigger, new StaticMessageProvider([$message], strtr(substr(base64_encode(hash('xxh128', serialize($message), true)), 0, 7), '/+', '._'), -7), $description));
Copy link
Member

Choose a reason for hiding this comment

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

FYI, there was a syntax error on this line. I fixed that in a45f6cb. Please double-check that I didn't mess anything with the logic implemented here.

This was referenced Jan 31, 2024
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.

[Scheduler] Cannot have multiple #[AsPeriodicTask] on the same method with the same frequency
6 participants