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

[Messenger] Introduce SelfStampableInterface #54366

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

Conversation

VincentLanglet
Copy link
Contributor

Q A
Branch? 7.1
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)];
     }
}

@ro0NL
Copy link
Contributor

ro0NL 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());

@VincentLanglet
Copy link
Contributor Author

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

@ro0NL
Copy link
Contributor

ro0NL commented Mar 21, 2024

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

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

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.

None yet

6 participants