-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Introduce SelfStampableInterface
#54366
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
base: 7.3
Are you sure you want to change the base?
Conversation
2e50e03
to
e385324
Compare
src/Symfony/Component/Messenger/Tests/Fixtures/SelfStampableDummyMessage.php
Outdated
Show resolved
Hide resolved
An alternative idea, also suggested by the comment that you've linked, was to use attributes for stamps. Can you elaborate why you dismissed that idea in favor of an interface? |
I misunderstood/forgot about the comment when working on this and the only idea I had in mind was an interface. Then, I reread the comment and even found #50812 IMHO the interface is simpler:
Also, dynamic stamps would be impossible with attribute. This would be useful for the LockStamp in #54141,
|
Sorry, I don't understand how will be solved, in the simple way, the class Message implements SelfStampableInterface
{
public function __construct(private int $projectId) {}
// With this stamp, I ensure there is never 2 messages for the same Id in the queue
public function getStamps(): array {
return [new LockStamp(self::class . $this->projectId)];
}
} situation with #[LockStamp()] // Error missing first param.
class Message implements SelfStampableInterface
{
public function __construct(private int $projectId) {}
} |
With a method, you also can add the stamp conditionnaly
@derrabus do you have time for a new look ? :) |
Maybe you have some time for a review @nicolas-grekas ? |
Friendly ping @derrabus ; would you have time for a review ? |
Unfortunately not at the moment. 😞 |
Looking at this after some time has passed make me think that stamps could simply be attributes on classes, and this would actually make this feature real. All it need it that all stamps extend Except maybe that it wouldn't allow dynamism in stamp data, do you have any real life use case for such dynamism? |
Stamps themselves should not be attributes, as they are metadata about a particular message, not about the class. For instance, there are some stamps that are added based on how the message is dispatched or how it is read from the queue. |
@ro0NL for applying automatic stamps on messages using a given class, I'd rather have a |
This is a big loss for me. I have some stamp like "DeduplicateStamp" or "DelayStamp" which are added by default (because used 99% of the time) but I need the ability to skip them sometimes. I have also stamp which are added based on the size of the data. This PR doesn't forbid stamp as attribute, it can still be done in another PR by someone else. |
Definitely agree, stamps are basically no more no less than very simple DTOs, exactly what attributes also are in the end. Whereas @stof is right about the fact that some stamps shouldn't be, my opinion is that those which can should directly be attributes. There's no need add an indirection layer here: the most direct implementation would also be the most readable in the end. |
OK, I'll stop talking now about attributes, this is not what the issue is about, thanks for answering. |
SelfStampableInterface
@@ -54,6 +55,10 @@ public function getIterator(): \Traversable | ||
|
||
public function dispatch(object $message, array $stamps = []): Envelope | ||
{ | ||
if ($message instanceof SelfStampableInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find a better place to automatically add the stamp
since it's the place where
$envelope = Envelope::wrap($message, $stamps);
is done.
And then return $middlewareIterator->current()->handle($envelope, $stack);
is done.
But if I introduce a SelfStampableMiddleware
to automatically add the stamp, I cannot be sure:
- The middleware is used
- The middleware is the first one
So the same issue would kinda occurs no ?
d1653e3
to
7a88877
Compare
d36a59e
to
7ae6408
Compare
7ae6408
to
8f0da1c
Compare
Idea was kinda suggested from #54141 (comment)
This interface would allow to define stamp I always want by default with a message.
By merging in the order
I can still override a stamp when dispatching the message.