Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Open
wants to merge 2 commits into
base: 7.3
Choose a base branch
Loading
from

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Mar 21, 2024

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

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

array_merge($message->getStamps(), $stamps);

I can still override a stamp when dispatching the message.

@carsonbot carsonbot changed the title Introduce SelfStampableInterface [Messenger] Introduce SelfStampableInterface Mar 21, 2024
@derrabus
Copy link
Member

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?

@VincentLanglet
Copy link
Contributor Author

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
but I'm not sure what would be the interest of attributes here.

IMHO the interface is simpler:

  • A feature with #[DelayStamp(123)] would require to declare every stamps as attributes, you cannot use this for other libs if they don't declare their stamps as attribute.
  • A feature #[AutoStamp(new DelayStamp(123))] solve the previous issue but It still require extra manipulation with ReflectionClass when the instanceof SelfStampableInterface and a getter does all the job.

Also, dynamic stamps would be impossible with attribute.

This would be useful for the LockStamp in #54141,
you could have

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)];
     }
}

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Mar 21, 2024

i tend to prefer stamps as attributes for the sake of decoupling

having runtime flexibility is nice, but IMHO it needs to be scalable, eg. we might want to use additional services to compute a stamp

in that sense, middleware layer already brings full runtime flexibility in userland to apply default stamps

for compiling stamps as-a-service from attributes, take this:

$stampServiceDef = new Definition($attr->getName(), $attr->getArguments());

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) {}
}

@VincentLanglet VincentLanglet requested a review from derrabus March 21, 2024 17:07
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@VincentLanglet
Copy link
Contributor Author

perhaps self-stamping is the way to go for such simple runtime usecases

no strong opinion :)

#[LockStamp()]

it could compute a key from the message payload by default

With a method, you also can add the stamp conditionnaly

public function getStamps(): array {
         return $this->foo ? [new FooStamp()] : [new BarStamp()];
     }

@derrabus do you have time for a new look ? :)

@VincentLanglet
Copy link
Contributor Author

Maybe you have some time for a review @nicolas-grekas ?

@VincentLanglet
Copy link
Contributor Author

Friendly ping @derrabus ; would you have time for a review ?

@derrabus
Copy link
Member

derrabus commented Nov 6, 2024

Unfortunately not at the moment. 😞

@pounard
Copy link
Contributor

pounard commented Nov 6, 2024

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 \Attribute and some outer middleware exist in the dispatcher to expand those as stamps.

Except maybe that it wouldn't allow dynamism in stamp data, do you have any real life use case for such dynamism?

@stof
Copy link
Member

stof commented Nov 7, 2024

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.

@stof
Copy link
Member

stof commented Nov 7, 2024

@ro0NL for applying automatic stamps on messages using a given class, I'd rather have a AutoStamp attribute (name to be bikeshed) taking a Stamp as argument (allowing to repeat the attribute) than making stamps themselves as attributes while stamps themselves cannot always be attributes. It would reduce confusion IMO.

@VincentLanglet
Copy link
Contributor Author

Except maybe that it wouldn't allow dynamism in stamp data, do you have any real life use case for such dynamism?

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.

@pounard
Copy link
Contributor

pounard commented Nov 7, 2024

stamps could be attributes, if the metadata is constant. Otherwise dont apply the stamp as an attribute on your message.

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.

@pounard
Copy link
Contributor

pounard commented Nov 7, 2024

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.

OK, I'll stop talking now about attributes, this is not what the issue is about, thanks for answering.

@OskarStark OskarStark changed the title [Messenger] Introduce SelfStampableInterface [Messenger] Introduce SelfStampableInterface Nov 7, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@VincentLanglet
Copy link
Contributor Author

Hi @derrabus @ro0NL @stof

It's unclear to me how to move this forward.
Stamp as attribute is a nice feature but should be done in another PR as it's unrealted.
Therefor I don't think there is a current request change here ; it might just require your review then ?

@@ -54,6 +55,10 @@ public function getIterator(): \Traversable

public function dispatch(object $message, array $stamps = []): Envelope
{
if ($message instanceof SelfStampableInterface) {
Copy link
Contributor Author

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 ?

@VincentLanglet VincentLanglet force-pushed the selfStambableInterface branch 4 times, most recently from d36a59e to 7ae6408 Compare February 7, 2025 09:48
@VincentLanglet
Copy link
Contributor Author

Hi @ro0NL @derrabus I rework the PR to use a middleware, if you want to review again.

Also I wonder if you have a better name in minde than SelfStampableInterface and Add SelfStampableStampMiddleware.

@VincentLanglet VincentLanglet force-pushed the selfStambableInterface branch from 7ae6408 to 8f0da1c Compare March 19, 2025 14:32
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.

8 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.