Skip to content

[Messenger] collect all stamps added on Envelope as collections #29159

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

Merged
merged 1 commit into from
Nov 12, 2018
Merged

[Messenger] collect all stamps added on Envelope as collections #29159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Symfony/Component/Messenger/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ CHANGELOG
* `ActivationMiddlewareDecorator` has been renamed `ActivationMiddleware`
* `AllowNoHandlerMiddleware` has been removed in favor of a new constructor argument on `HandleMessageMiddleware`
* The `ContainerHandlerLocator`, `AbstractHandlerLocator`, `SenderLocator` and `AbstractSenderLocator` classes have been removed
* `Envelope::all()` takes a new optional `$stampFqcn` argument and returns the stamps for the specified FQCN, or all stamps by their class name
* `Envelope::get()` has been renamed `Envelope::last()`

4.1.0
-----
Expand Down
16 changes: 10 additions & 6 deletions src/Symfony/Component/Messenger/Envelope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public function __construct($message, StampInterface ...$stamps)
$this->message = $message;

foreach ($stamps as $stamp) {
$this->stamps[\get_class($stamp)] = $stamp;
$this->stamps[\get_class($stamp)][] = $stamp;
}
}

Expand All @@ -48,22 +48,26 @@ public function with(StampInterface ...$stamps): self
$cloned = clone $this;

foreach ($stamps as $stamp) {
$cloned->stamps[\get_class($stamp)] = $stamp;
$cloned->stamps[\get_class($stamp)][] = $stamp;
}

return $cloned;
}

public function get(string $stampFqcn): ?StampInterface
public function last(string $stampFqcn): ?StampInterface
{
return $this->stamps[$stampFqcn] ?? null;
return isset($this->stamps[$stampFqcn]) ? end($this->stamps[$stampFqcn]) : null;
}

/**
* @return StampInterface[] indexed by fqcn
* @return StampInterface[]|StampInterface[][] The stamps for the specified FQCN, or all stamps by their class name
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to have 2 different methods instead of two kind of returned value?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure personally: when reading, all() or all(SomeStampp:class) is very clear to me. Discoverability looks better also (same as EventDispatcher::getListeners() btw) + types-wise, the interface has no method so autocompletion won't help.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're both right.
Having one method is fine, but isn't grouping them by FQCN in one case a relic from how we used to manage stamps of the same type before ?
If now stamps should also give us an history of the life of the message, it would feel more natural if all() (without argument) did just return a StampInterface[] with all the stamps in the order they were added (ie: not grouped by FQCN anymore).

*/
public function all(): array
public function all(string $stampFqcn = null): array
{
if (null !== $stampFqcn) {
return $this->stamps[$stampFqcn] ?? array();
}

return $this->stamps;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Messenger/Middleware/SendMessageMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function __construct(SendersLocatorInterface $sendersLocator)
*/
public function handle(Envelope $envelope, StackInterface $stack): Envelope
{
if ($envelope->get(ReceivedStamp::class)) {
if ($envelope->all(ReceivedStamp::class)) {
// it's a received message, do not send it back
return $stack->next()->handle($envelope, $stack);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Messenger/Middleware/ValidationMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function handle(Envelope $envelope, StackInterface $stack): Envelope
$message = $envelope->getMessage();
$groups = null;
/** @var ValidationStamp|null $validationStamp */
if ($validationStamp = $envelope->get(ValidationStamp::class)) {
if ($validationStamp = $envelope->last(ValidationStamp::class)) {
$groups = $validationStamp->getGroups();
}

Expand Down
12 changes: 6 additions & 6 deletions src/Symfony/Component/Messenger/Tests/EnvelopeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function testConstruct()

$this->assertSame($dummy, $envelope->getMessage());
$this->assertArrayHasKey(ReceivedStamp::class, $stamps = $envelope->all());
$this->assertSame($receivedStamp, $stamps[ReceivedStamp::class]);
$this->assertSame($receivedStamp, $stamps[ReceivedStamp::class][0]);
}

public function testWithReturnsNewInstance()
Expand All @@ -39,13 +39,13 @@ public function testWithReturnsNewInstance()
$this->assertNotSame($envelope, $envelope->with(new ReceivedStamp()));
}

public function testGet()
public function testGetLast()
{
$receivedStamp = new ReceivedStamp();
$envelope = new Envelope($dummy = new DummyMessage('dummy'), $receivedStamp);

$this->assertSame($receivedStamp, $envelope->get(ReceivedStamp::class));
$this->assertNull($envelope->get(ValidationStamp::class));
$this->assertSame($receivedStamp, $envelope->last(ReceivedStamp::class));
$this->assertNull($envelope->last(ValidationStamp::class));
}

public function testAll()
Expand All @@ -57,8 +57,8 @@ public function testAll()

$stamps = $envelope->all();
$this->assertArrayHasKey(ReceivedStamp::class, $stamps);
$this->assertSame($receivedStamp, $stamps[ReceivedStamp::class]);
$this->assertSame($receivedStamp, $stamps[ReceivedStamp::class][0]);
$this->assertArrayHasKey(ValidationStamp::class, $stamps);
$this->assertSame($validationStamp, $stamps[ValidationStamp::class]);
$this->assertSame($validationStamp, $stamps[ValidationStamp::class][0]);
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/Messenger/Tests/TraceableMessageBusTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public function testItTracesDispatchWithEnvelope()
$this->assertCount(1, $tracedMessages = $traceableBus->getDispatchedMessages());
$this->assertArraySubset(array(
'message' => $message,
'stamps' => array($stamp),
'stamps' => array(array($stamp)),
'caller' => array(
'name' => 'TraceableMessageBusTest.php',
'file' => __FILE__,
Expand Down
23 changes: 6 additions & 17 deletions src/Symfony/Component/Messenger/Tests/Transport/Serialization/SerializerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,13 @@
use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;
use Symfony\Component\Messenger\Transport\Serialization\Serializer;
use Symfony\Component\Serializer as SerializerComponent;
use Symfony\Component\Serializer\Encoder\JsonEncoder;
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;

class SerializerTest extends TestCase
{
public function testEncodedIsDecodable()
{
$serializer = new Serializer(
new SerializerComponent\Serializer(array(new ObjectNormalizer()), array('json' => new JsonEncoder()))
);
$serializer = new Serializer();

$envelope = new Envelope(new DummyMessage('Hello'));

Expand All @@ -36,9 +33,7 @@ public function testEncodedIsDecodable()

public function testEncodedWithStampsIsDecodable()
{
$serializer = new Serializer(
new SerializerComponent\Serializer(array(new ObjectNormalizer()), array('json' => new JsonEncoder()))
);
$serializer = new Serializer();

$envelope = (new Envelope(new DummyMessage('Hello')))
->with(new SerializerStamp(array(ObjectNormalizer::GROUPS => array('foo'))))
Expand All @@ -50,9 +45,7 @@ public function testEncodedWithStampsIsDecodable()

public function testEncodedIsHavingTheBodyAndTypeHeader()
{
$serializer = new Serializer(
new SerializerComponent\Serializer(array(new ObjectNormalizer()), array('json' => new JsonEncoder()))
);
$serializer = new Serializer();

$encoded = $serializer->encode(new Envelope(new DummyMessage('Hello')));

Expand Down Expand Up @@ -81,11 +74,7 @@ public function testUsesTheCustomFormatAndContext()

public function testEncodedWithSymfonySerializerForStamps()
{
$serializer = new Serializer(
new SerializerComponent\Serializer(array(new ObjectNormalizer()), array('json' => new JsonEncoder())),
'json',
array()
);
$serializer = new Serializer();

$envelope = (new Envelope(new DummyMessage('Hello')))
->with($serializerStamp = new SerializerStamp(array(ObjectNormalizer::GROUPS => array('foo'))))
Expand All @@ -102,7 +91,7 @@ public function testEncodedWithSymfonySerializerForStamps()

$decoded = $serializer->decode($encoded);

$this->assertEquals($serializerStamp, $decoded->get(SerializerStamp::class));
$this->assertEquals($validationStamp, $decoded->get(ValidationStamp::class));
$this->assertEquals($serializerStamp, $decoded->last(SerializerStamp::class));
$this->assertEquals($validationStamp, $decoded->last(ValidationStamp::class));
}
}
3 changes: 2 additions & 1 deletion src/Symfony/Component/Messenger/Tests/WorkerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,11 @@ public function testWorkerDispatchTheReceivedMessage()

public function testWorkerDoesNotWrapMessagesAlreadyWrappedWithReceivedMessage()
{
$envelope = (new Envelope(new DummyMessage('API')))->with(new ReceivedStamp());
$envelope = new Envelope(new DummyMessage('API'));
$receiver = new CallbackReceiver(function ($handler) use ($envelope) {
$handler($envelope);
});
$envelope = $envelope->with(new ReceivedStamp());

$bus = $this->getMockBuilder(MessageBusInterface::class)->getMock();
$bus->expects($this->at(0))->method('dispatch')->with($envelope)->willReturn($envelope);
Expand Down
25 changes: 14 additions & 11 deletions src/Symfony/Component/Messenger/Transport/Serialization/Serializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Symfony\Component\Messenger\Stamp\SerializerStamp;
use Symfony\Component\Serializer\Encoder\JsonEncoder;
use Symfony\Component\Serializer\Encoder\XmlEncoder;
use Symfony\Component\Serializer\Normalizer\ArrayDenormalizer;
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;
use Symfony\Component\Serializer\Serializer as SymfonySerializer;
use Symfony\Component\Serializer\SerializerInterface as SymfonySerializerInterface;
Expand All @@ -34,9 +35,9 @@ class Serializer implements SerializerInterface
private $format;
private $context;

public function __construct(SymfonySerializerInterface $serializer, string $format = 'json', array $context = array())
public function __construct(SymfonySerializerInterface $serializer = null, string $format = 'json', array $context = array())
{
$this->serializer = $serializer;
$this->serializer = $serializer ?? self::create()->serializer;
$this->format = $format;
$this->context = $context;
}
Expand All @@ -48,7 +49,7 @@ public static function create(): self
}

$encoders = array(new XmlEncoder(), new JsonEncoder());
$normalizers = array(new ObjectNormalizer());
$normalizers = array(new ArrayDenormalizer(), new ObjectNormalizer());
$serializer = new SymfonySerializer($normalizers, $encoders);

return new self($serializer);
Expand All @@ -70,9 +71,8 @@ public function decode(array $encodedEnvelope): Envelope
$stamps = $this->decodeStamps($encodedEnvelope);

$context = $this->context;
/** @var SerializerStamp|null $serializerStamp */
if ($serializerStamp = $stamps[SerializerStamp::class] ?? null) {
$context = $serializerStamp->getContext() + $context;
if (isset($stamps[SerializerStamp::class])) {
$context = end($stamps[SerializerStamp::class])->getContext() + $context;
Copy link
Contributor

Choose a reason for hiding this comment

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

we can actually support multiple stamps no?

Copy link
Member Author

Choose a reason for hiding this comment

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

like merging all stamps together? I don't know, so I wouldn't change that in this PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

i think deciding on end() vs reset() vs merging kinda belongs to this PR

Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

about reset() vs end(): the previous behavior returned the latest stamp, so it should be end() - in need of any reason to change it I'd keep the behavior here.

}

$message = $this->serializer->deserialize($encodedEnvelope['body'], $encodedEnvelope['headers']['type'], $this->format, $context);
Expand All @@ -87,7 +87,7 @@ public function encode(Envelope $envelope): array
{
$context = $this->context;
/** @var SerializerStamp|null $serializerStamp */
if ($serializerStamp = $envelope->get(SerializerStamp::class)) {
if ($serializerStamp = $envelope->last(SerializerStamp::class)) {
$context = $serializerStamp->getContext() + $context;
}

Expand All @@ -107,21 +107,24 @@ private function decodeStamps(array $encodedEnvelope): array
continue;
}

$stamps[] = $this->serializer->deserialize($value, substr($name, \strlen(self::STAMP_HEADER_PREFIX)), $this->format, $this->context);
$stamps[] = $this->serializer->deserialize($value, substr($name, \strlen(self::STAMP_HEADER_PREFIX)).'[]', $this->format, $this->context);
}
if ($stamps) {
$stamps = array_merge(...$stamps);
}

return $stamps;
}

private function encodeStamps(Envelope $envelope): array
{
if (!$stamps = $envelope->all()) {
if (!$allStamps = $envelope->all()) {
return array();
}

$headers = array();
foreach ($stamps as $stamp) {
$headers[self::STAMP_HEADER_PREFIX.\get_class($stamp)] = $this->serializer->serialize($stamp, $this->format, $this->context);
foreach ($allStamps as $class => $stamps) {
$headers[self::STAMP_HEADER_PREFIX.$class] = $this->serializer->serialize($stamps, $this->format, $this->context);
}

return $headers;
Expand Down