Skip to content
Permalink
Browse files

minor #31389 [Messenger] fix wrong use of generator returns (Tobion)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] fix wrong use of generator returns

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

I've seen this problem many times: Mixing `yield` with `return []`.
Unfortunately it cannot be forbidden at the compiler level because it's actually a feature: https://www.php.net/manual/de/generator.getreturn.php
But usually not intended that way.
Also added some other minor cleanups I've found.

Commits
-------

e8a09e9 [Messenger] fix wrong use of generator returns
  • Loading branch information...
fabpot committed May 6, 2019
2 parents 660b18f + e8a09e9 commit bee5216ae86384ac6517c499ba0b16103c0501bb
@@ -32,7 +32,7 @@ public function __construct(string $exceptionMessage, string $originalReceiverNa
$this->exceptionMessage = $exceptionMessage;
$this->originalReceiverName = $originalReceiverName;
$this->flattenException = $flattenException;
$this->sentAt = new \DateTime();
$this->sentAt = new \DateTimeImmutable();
}
public function getExceptionMessage(): string
@@ -50,7 +50,7 @@ public function getFlattenException(): ?FlattenException
return $this->flattenException;
}
public function getSentAt(): \DateTime
public function getSentAt(): \DateTimeInterface
{
return $this->sentAt;
}
@@ -28,6 +28,6 @@ public function testGetters()
$this->assertSame('exception message', $stamp->getExceptionMessage());
$this->assertSame('original_receiver', $stamp->getOriginalReceiverName());
$this->assertSame($flattenException, $stamp->getFlattenException());
$this->assertInstanceOf(\DateTime::class, $stamp->getSentAt());
$this->assertInstanceOf(\DateTimeInterface::class, $stamp->getSentAt());
}
}
@@ -36,7 +36,7 @@ public function testItReturnsTheDecodedMessageToTheHandler()
$connection->method('get')->willReturn($doctrineEnvelope);
$receiver = new DoctrineReceiver($connection, $serializer);
$actualEnvelopes = iterator_to_array($receiver->get());
$actualEnvelopes = $receiver->get();
$this->assertCount(1, $actualEnvelopes);
/** @var Envelope $actualEnvelope */
$actualEnvelope = $actualEnvelopes[0];
@@ -67,7 +67,7 @@ public function testItRejectTheMessageIfThereIsAMessageDecodingFailedException()
$connection->expects($this->once())->method('reject');
$receiver = new DoctrineReceiver($connection, $serializer);
iterator_to_array($receiver->get());
$receiver->get();
}
public function testAll()
@@ -46,7 +46,7 @@ public function testReceivesMessages()
$serializer->method('decode')->with(['body' => 'body', 'headers' => ['my' => 'header']])->willReturn(new Envelope($decodedMessage));
$connection->method('get')->willReturn($doctrineEnvelope);
$envelopes = iterator_to_array($transport->get());
$envelopes = $transport->get();
$this->assertSame($decodedMessage, $envelopes[0]->getMessage());
}
@@ -13,12 +13,8 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;
use Symfony\Component\Messenger\Tests\Fixtures\SecondMessage;
use Symfony\Component\Messenger\Transport\Receiver\ReceiverInterface;
use Symfony\Component\Messenger\Transport\Receiver\SingleMessageReceiver;
use Symfony\Component\Messenger\Transport\Sender\SenderInterface;
use Symfony\Component\Messenger\Transport\Sender\SendersLocator;
class SingleMessageReceiverTest extends TestCase
{
@@ -28,11 +24,11 @@ public function testItReceivesOnlyOneMessage()
$envelope = new Envelope(new \stdClass());
$receiver = new SingleMessageReceiver($innerReceiver, $envelope);
$received = \iterator_to_array($receiver->get());
$received = $receiver->get();
$this->assertCount(1, $received);
$this->assertSame($received[0], $envelope);
$this->assertEmpty(\iterator_to_array($receiver->get()));
$this->assertEmpty($receiver->get());
}
public function testCallsAreForwarded()
@@ -33,7 +33,7 @@ public function testItReturnsTheDecodedMessageToTheHandler()
$connection->method('get')->willReturn($redisEnvelop);
$receiver = new RedisReceiver($connection, $serializer);
$actualEnvelopes = iterator_to_array($receiver->get());
$actualEnvelopes = $receiver->get();
$this->assertCount(1, $actualEnvelopes);
$this->assertEquals(new DummyMessage('Hi'), $actualEnvelopes[0]->getMessage());
}
@@ -51,7 +51,7 @@ public function testItRejectTheMessageIfThereIsAMessageDecodingFailedException()
$connection->expects($this->once())->method('reject');
$receiver = new RedisReceiver($connection, $serializer);
iterator_to_array($receiver->get());
$receiver->get();
}
private function createRedisEnvelope()
@@ -46,7 +46,7 @@ public function testReceivesMessages()
$serializer->method('decode')->with(['body' => 'body', 'headers' => ['my' => 'header']])->willReturn(new Envelope($decodedMessage));
$connection->method('get')->willReturn($redisEnvelope);
$envelopes = iterator_to_array($transport->get());
$envelopes = $transport->get();
$this->assertSame($decodedMessage, $envelopes[0]->getMessage());
}
@@ -57,7 +57,7 @@ private function getEnvelope(string $queueName): iterable
}
if (null === $amqpEnvelope) {
return [];
return;
}
try {
@@ -84,12 +84,12 @@ public function getMessageCount(): int
return ($this->receiver ?? $this->getReceiver())->getMessageCount();
}
private function getReceiver()
private function getReceiver(): AmqpReceiver
{
return $this->receiver = new AmqpReceiver($this->connection, $this->serializer);
}
private function getSender()
private function getSender(): AmqpSender
{
return $this->sender = new AmqpSender($this->connection, $this->serializer);
}
@@ -54,7 +54,7 @@ public function get(): iterable
return [];
}
yield $this->createEnvelopeFromData($doctrineEnvelope);
return [$this->createEnvelopeFromData($doctrineEnvelope)];
}
/**
@@ -1,5 +1,14 @@
<?php
/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace Symfony\Component\Messenger\Transport\Receiver;
use Symfony\Component\Messenger\Envelope;
@@ -1,5 +1,14 @@
<?php
/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace Symfony\Component\Messenger\Transport\Receiver;
use Symfony\Component\Messenger\Envelope;
@@ -32,7 +41,7 @@ public function get(): iterable
$this->hasReceived = true;
yield $this->envelope;
return [$this->envelope];
}
public function ack(Envelope $envelope): void
@@ -57,7 +57,7 @@ public function get(): iterable
throw $exception;
}
yield $envelope->with(new RedisReceivedStamp($redisEnvelope['id']));
return [$envelope->with(new RedisReceivedStamp($redisEnvelope['id']))];
}
/**
@@ -76,12 +76,12 @@ public function setup(): void
$this->connection->setup();
}
private function getReceiver()
private function getReceiver(): RedisReceiver
{
return $this->receiver = new RedisReceiver($this->connection, $this->serializer);
}
private function getSender()
private function getSender(): RedisSender
{
return $this->sender = new RedisSender($this->connection, $this->serializer);
}
@@ -136,16 +136,11 @@ private function handleMessage(Envelope $envelope, ReceiverInterface $receiver,
$envelope = $throwable->getEnvelope();
}
$shouldRetry = $this->shouldRetry($throwable, $envelope, $retryStrategy);
$shouldRetry = $retryStrategy && $this->shouldRetry($throwable, $envelope, $retryStrategy);
$this->dispatchEvent(new WorkerMessageFailedEvent($envelope, $transportName, $throwable, $shouldRetry));
if ($shouldRetry) {
if (null === $retryStrategy) {
// not logically allowed, but check just in case
throw new LogicException('Retrying is not supported without a retry strategy.');
}
$retryCount = $this->getRetryCount($envelope) + 1;
if (null !== $this->logger) {
$this->logger->error('Retrying {class} - retry #{retryCount}.', $context + ['retryCount' => $retryCount, 'error' => $throwable]);
@@ -194,16 +189,12 @@ private function dispatchEvent($event)
$this->eventDispatcher->dispatch($event);
}
private function shouldRetry(\Throwable $e, Envelope $envelope, ?RetryStrategyInterface $retryStrategy): bool
private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInterface $retryStrategy): bool
{
if ($e instanceof UnrecoverableMessageHandlingException) {
return false;
}
if (null === $retryStrategy) {
return false;
}
$sentStamp = $envelope->last(SentStamp::class);
if (null === $sentStamp) {
if (null !== $this->logger) {
@@ -59,7 +59,7 @@ public function stop(): void
$this->decoratedWorker->stop();
}
private function shouldRestart(float $workerStartedAt)
private function shouldRestart(float $workerStartedAt): bool
{
$cacheItem = $this->cachePool->getItem(self::RESTART_REQUESTED_TIMESTAMP_KEY);

0 comments on commit bee5216

Please sign in to comment.
You can’t perform that action at this time.