From 0525a8819db53bc66c878413829a4368d63c3d47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateus=20Guimar=C3=A3es?= Date: Sun, 29 Oct 2023 21:59:22 -0300 Subject: [PATCH] [10.x] Dispatch events based on a DB transaction result (#48705) * wip * Refactor * Add EventFake support * remove strict types * Make styleCI happy * Fix test * Add missing test for EventFake * Add test to handle nested transactions * fix typo * Make styleci happy * formatting, inject manager resolver * formatting * formatting * formatting * formatting * more thorough solution * Add additional test for nested transactions * Add additional nested transaction test --------- Co-authored-by: Taylor Otwell --- .../Events/ShouldDispatchAfterCommit.php | 8 + .../Events/ShouldHandleEventsAfterCommit.php | 8 + .../Queue/ShouldQueueAfterCommit.php | 8 + .../Eloquent/BroadcastsEventsAfterCommit.php | 18 ++ src/Illuminate/Events/Dispatcher.php | 79 ++++++++- .../Events/EventServiceProvider.php | 4 + src/Illuminate/Mail/SendQueuedMailable.php | 8 +- .../Notifications/SendQueuedNotifications.php | 9 +- src/Illuminate/Queue/Queue.php | 5 + .../Support/Testing/Fakes/EventFake.php | 22 ++- tests/Integration/Events/EventFakeTest.php | 60 +++++++ .../ShouldDispatchAfterCommitEventTest.php | 163 ++++++++++++++++++ 12 files changed, 383 insertions(+), 9 deletions(-) create mode 100644 src/Illuminate/Contracts/Events/ShouldDispatchAfterCommit.php create mode 100644 src/Illuminate/Contracts/Events/ShouldHandleEventsAfterCommit.php create mode 100644 src/Illuminate/Contracts/Queue/ShouldQueueAfterCommit.php create mode 100644 src/Illuminate/Database/Eloquent/BroadcastsEventsAfterCommit.php create mode 100644 tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php diff --git a/src/Illuminate/Contracts/Events/ShouldDispatchAfterCommit.php b/src/Illuminate/Contracts/Events/ShouldDispatchAfterCommit.php new file mode 100644 index 000000000000..8f1fbdd4d697 --- /dev/null +++ b/src/Illuminate/Contracts/Events/ShouldDispatchAfterCommit.php @@ -0,0 +1,8 @@ +parseEventAndPayload( - $event, $payload - ); + [$isEventObject, $event, $payload] = [ + is_object($event), + ...$this->parseEventAndPayload($event, $payload) + ]; + + // If the event is not intended to be dispatched unless the current database + // transaction is successful, we'll register a callback which will handle + // dispatching this event on the next successful DB transaction commit. + if ($isEventObject && + $payload[0] instanceof ShouldDispatchAfterCommit && + ! is_null($transactions = $this->resolveTransactionManager())) { + $transactions->addCallback( + fn () => $this->invokeListeners($event, $payload, $halt) + ); + return null; + } + + return $this->invokeListeners($event, $payload, $halt); + } + + /** + * Broadcast an event and call its listeners. + * + * @param string|object $event + * @param mixed $payload + * @param bool $halt + * @return array|null + */ + protected function invokeListeners($event, $payload, $halt = false) + { if ($this->shouldBroadcast($payload)) { $this->broadcastEvent($payload[0]); } @@ -525,7 +562,9 @@ protected function createQueuedHandlerCallable($class, $method) */ protected function handlerShouldBeDispatchedAfterDatabaseTransactions($listener) { - return ($listener->afterCommit ?? null) && $this->container->bound('db.transactions'); + return (($listener->afterCommit ?? null) || + $listener instanceof ShouldHandleEventsAfterCommit) && + $this->resolveTransactionManager(); } /** @@ -540,7 +579,7 @@ protected function createCallbackForListenerRunningAfterCommits($listener, $meth return function () use ($method, $listener) { $payload = func_get_args(); - $this->container->make('db.transactions')->addCallback( + $this->resolveTransactionManager()->addCallback( function () use ($listener, $method, $payload) { $listener->$method(...$payload); } @@ -624,7 +663,12 @@ protected function propagateListenerOptions($listener, $job) return tap($job, function ($job) use ($listener) { $data = array_values($job->data); - $job->afterCommit = property_exists($listener, 'afterCommit') ? $listener->afterCommit : null; + if ($listener instanceof ShouldQueueAfterCommit) { + $job->afterCommit = true; + } else { + $job->afterCommit = property_exists($listener, 'afterCommit') ? $listener->afterCommit : null; + } + $job->backoff = method_exists($listener, 'backoff') ? $listener->backoff(...$data) : ($listener->backoff ?? null); $job->maxExceptions = $listener->maxExceptions ?? null; $job->retryUntil = method_exists($listener, 'retryUntil') ? $listener->retryUntil(...$data) : null; @@ -697,6 +741,29 @@ public function setQueueResolver(callable $resolver) return $this; } + /** + * Get the database transaction manager implementation from the resolver. + * + * @return \Illuminate\Database\DatabaseTransactionsManager|null + */ + protected function resolveTransactionManager() + { + return call_user_func($this->transactionManagerResolver); + } + + /** + * Set the database transaction manager resolver implementation. + * + * @param callable $resolver + * @return $this + */ + public function setTransactionManagerResolver(callable $resolver) + { + $this->transactionManagerResolver = $resolver; + + return $this; + } + /** * Gets the raw, unprepared listeners. * diff --git a/src/Illuminate/Events/EventServiceProvider.php b/src/Illuminate/Events/EventServiceProvider.php index 15fb60b10bba..cf9fbe25e3c3 100755 --- a/src/Illuminate/Events/EventServiceProvider.php +++ b/src/Illuminate/Events/EventServiceProvider.php @@ -17,6 +17,10 @@ public function register() $this->app->singleton('events', function ($app) { return (new Dispatcher($app))->setQueueResolver(function () use ($app) { return $app->make(QueueFactoryContract::class); + })->setTransactionManagerResolver(function () use ($app) { + return $app->bound('db.transactions') + ? $app->make('db.transactions') + : null; }); }); } diff --git a/src/Illuminate/Mail/SendQueuedMailable.php b/src/Illuminate/Mail/SendQueuedMailable.php index 28a72c47c7de..b9fec9d03849 100644 --- a/src/Illuminate/Mail/SendQueuedMailable.php +++ b/src/Illuminate/Mail/SendQueuedMailable.php @@ -6,6 +6,7 @@ use Illuminate\Contracts\Mail\Factory as MailFactory; use Illuminate\Contracts\Mail\Mailable as MailableContract; use Illuminate\Contracts\Queue\ShouldBeEncrypted; +use Illuminate\Contracts\Queue\ShouldQueueAfterCommit; use Illuminate\Queue\InteractsWithQueue; class SendQueuedMailable @@ -57,7 +58,12 @@ public function __construct(MailableContract $mailable) { $this->mailable = $mailable; - $this->afterCommit = property_exists($mailable, 'afterCommit') ? $mailable->afterCommit : null; + if ($mailable instanceof ShouldQueueAfterCommit) { + $this->afterCommit = true; + } else { + $this->afterCommit = property_exists($mailable, 'afterCommit') ? $mailable->afterCommit : null; + } + $this->connection = property_exists($mailable, 'connection') ? $mailable->connection : null; $this->maxExceptions = property_exists($mailable, 'maxExceptions') ? $mailable->maxExceptions : null; $this->queue = property_exists($mailable, 'queue') ? $mailable->queue : null; diff --git a/src/Illuminate/Notifications/SendQueuedNotifications.php b/src/Illuminate/Notifications/SendQueuedNotifications.php index 19af18853667..f63d7bf9afe4 100644 --- a/src/Illuminate/Notifications/SendQueuedNotifications.php +++ b/src/Illuminate/Notifications/SendQueuedNotifications.php @@ -5,6 +5,7 @@ use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldBeEncrypted; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Contracts\Queue\ShouldQueueAfterCommit; use Illuminate\Database\Eloquent\Collection as EloquentCollection; use Illuminate\Database\Eloquent\Model; use Illuminate\Queue\InteractsWithQueue; @@ -80,7 +81,13 @@ public function __construct($notifiables, $notification, array $channels = null) $this->tries = property_exists($notification, 'tries') ? $notification->tries : null; $this->timeout = property_exists($notification, 'timeout') ? $notification->timeout : null; $this->maxExceptions = property_exists($notification, 'maxExceptions') ? $notification->maxExceptions : null; - $this->afterCommit = property_exists($notification, 'afterCommit') ? $notification->afterCommit : null; + + if ($notification instanceof ShouldQueueAfterCommit) { + $this->afterCommit = true; + } else { + $this->afterCommit = property_exists($notification, 'afterCommit') ? $notification->afterCommit : null; + } + $this->shouldBeEncrypted = $notification instanceof ShouldBeEncrypted; } diff --git a/src/Illuminate/Queue/Queue.php b/src/Illuminate/Queue/Queue.php index 03085f60a05d..0ce7ad1ac1ce 100755 --- a/src/Illuminate/Queue/Queue.php +++ b/src/Illuminate/Queue/Queue.php @@ -7,6 +7,7 @@ use Illuminate\Container\Container; use Illuminate\Contracts\Encryption\Encrypter; use Illuminate\Contracts\Queue\ShouldBeEncrypted; +use Illuminate\Contracts\Queue\ShouldQueueAfterCommit; use Illuminate\Queue\Events\JobQueued; use Illuminate\Support\Arr; use Illuminate\Support\InteractsWithTime; @@ -325,6 +326,10 @@ function () use ($payload, $queue, $delay, $callback, $job) { */ protected function shouldDispatchAfterCommit($job) { + if (is_object($job) && $job instanceof ShouldQueueAfterCommit) { + return true; + } + if (! $job instanceof Closure && is_object($job) && isset($job->afterCommit)) { return $job->afterCommit; } diff --git a/src/Illuminate/Support/Testing/Fakes/EventFake.php b/src/Illuminate/Support/Testing/Fakes/EventFake.php index 7a32315ce5d5..4a4fc7c5b22d 100644 --- a/src/Illuminate/Support/Testing/Fakes/EventFake.php +++ b/src/Illuminate/Support/Testing/Fakes/EventFake.php @@ -3,7 +3,9 @@ namespace Illuminate\Support\Testing\Fakes; use Closure; +use Illuminate\Container\Container; use Illuminate\Contracts\Events\Dispatcher; +use Illuminate\Contracts\Events\ShouldDispatchAfterCommit; use Illuminate\Support\Arr; use Illuminate\Support\Str; use Illuminate\Support\Traits\ForwardsCalls; @@ -297,7 +299,7 @@ public function dispatch($event, $payload = [], $halt = false) $name = is_object($event) ? get_class($event) : (string) $event; if ($this->shouldFakeEvent($name, $payload)) { - $this->events[$name][] = func_get_args(); + $this->fakeEvent($event, $name, func_get_args()); } else { return $this->dispatcher->dispatch($event, $payload, $halt); } @@ -329,6 +331,24 @@ protected function shouldFakeEvent($eventName, $payload) ->isNotEmpty(); } + /** + * Push the event onto the fake events array immediately or after the next database transaction. + * + * @param string|object $event + * @param string $name + * @param array $arguments + * @return void + */ + protected function fakeEvent($event, $name, $arguments) + { + if ($event instanceof ShouldDispatchAfterCommit && Container::getInstance()->bound('db.transactions')) { + return Container::getInstance()->make('db.transactions') + ->addCallback(fn () => $this->events[$name][] = $arguments); + } + + $this->events[$name][] = $arguments; + } + /** * Determine whether an event should be dispatched or not. * diff --git a/tests/Integration/Events/EventFakeTest.php b/tests/Integration/Events/EventFakeTest.php index 2d85bbd01274..a7e9b97cf096 100644 --- a/tests/Integration/Events/EventFakeTest.php +++ b/tests/Integration/Events/EventFakeTest.php @@ -3,12 +3,16 @@ namespace Illuminate\Tests\Integration\Events; use Closure; +use Exception; +use Illuminate\Contracts\Events\ShouldDispatchAfterCommit; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Arr; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Schema; use Orchestra\Testbench\TestCase; +use PHPUnit\Framework\ExpectationFailedException; class EventFakeTest extends TestCase { @@ -182,6 +186,57 @@ public function testMissingMethodsAreForwarded() $this->assertEquals('bar', Event::fake()->foo()); } + + public function testShouldDispatchAfterCommitEventsAreNotDispatchedIfTransactionFails() + { + Event::fake(); + + try { + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitEvent()); + + throw new Exception('foo'); + }); + } catch (Exception $e) { + } + + Event::assertNotDispatched(ShouldDispatchAfterCommitEvent::class); + } + + public function testShouldDispatchAfterCommitEventsAreDispatchedIfTransactionSucceeds() + { + Event::fake(); + + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitEvent()); + }); + + Event::assertDispatched(ShouldDispatchAfterCommitEvent::class); + } + + public function testShouldDispatchAfterCommitEventsAreDispatchedIfThereIsNoTransaction() + { + Event::fake(); + + Event::dispatch(new ShouldDispatchAfterCommitEvent()); + Event::assertDispatched(ShouldDispatchAfterCommitEvent::class); + } + + public function testAssertNothingDispatchedShouldDispatchAfterCommit() + { + Event::fake(); + Event::assertNothingDispatched(); + + Event::dispatch(new ShouldDispatchAfterCommitEvent); + Event::dispatch(new ShouldDispatchAfterCommitEvent); + + try { + Event::assertNothingDispatched(); + $this->fail(); + } catch (ExpectationFailedException $e) { + $this->assertStringContainsString('2 unexpected events were dispatched.', $e->getMessage()); + } + } } class Post extends Model @@ -248,3 +303,8 @@ public function __invoke($event) // } } + +class ShouldDispatchAfterCommitEvent implements ShouldDispatchAfterCommit +{ + // +} diff --git a/tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php b/tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php new file mode 100644 index 000000000000..6984f26a6a35 --- /dev/null +++ b/tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php @@ -0,0 +1,163 @@ +assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testEventIsNotDispatchedIfTransactionFails() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + + try { + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitTestEvent); + + throw new \Exception; + }); + } catch (\Exception) { + } + + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testEventIsDispatchedIfTransactionSucceeds() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitTestEvent); + }); + + $this->assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testItHandlesNestedTransactions() + { + // We are going to dispatch 2 different events in 2 different transactions. + // The parent transaction will succeed, but the nested transaction is going to fail and be rolled back. + // We want to ensure the event dispatched on the child transaction does not get published, since it failed, + // however, the event dispatched on the parent transaction should still be dispatched as usual. + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + Event::listen(AnotherShouldDispatchAfterCommitTestEvent::class, AnotherShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + try { + DB::transaction(function () { + // This event should not be dispatched since the transaction is going to fail. + Event::dispatch(new ShouldDispatchAfterCommitTestEvent); + throw new \Exception; + }); + } catch (\Exception) { + } + + // This event should be dispatched, as the parent transaction does not fail. + Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent); + }); + + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + $this->assertTrue(AnotherShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testItOnlyDispatchesNestedTransactionsEventsAfterTheRootTransactionIsCommitted() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + Event::listen(AnotherShouldDispatchAfterCommitTestEvent::class, AnotherShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent); + + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitTestEvent); + }); + + // Although the child transaction has been concluded, the parent transaction has not. + // The event dispatched on the child transaction should not have been dispatched. + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + $this->assertFalse(AnotherShouldDispatchAfterCommitTestEvent::$ran); + }); + + // Now that the parent transaction has been committed, the event + // on the child transaction should also have been dispatched. + $this->assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); + $this->assertTrue(AnotherShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testItOnlyDispatchesNestedTransactionsEventsAfterTheRootTransactionIsCommitedDifferentOrder() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + Event::listen(AnotherShouldDispatchAfterCommitTestEvent::class, AnotherShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitTestEvent); + }); + + // Although the child transaction has been concluded, the parent transaction has not. + // The event dispatched on the child transaction should not have been dispatched. + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + + // The main difference with this test is that we dispatch an event on the parent transaction + // at the end. This is important due to how the DatabaseTransactionsManager works. + Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent); + }); + + // Now that the parent transaction has been committed, the event + // on the child transaction should also have been dispatched. + $this->assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); + $this->assertTrue(AnotherShouldDispatchAfterCommitTestEvent::$ran); + } +} + +class TransactionUnawareTestEvent +{ + public static $ran = false; +} + +class ShouldDispatchAfterCommitTestEvent implements ShouldDispatchAfterCommit +{ + public static $ran = false; +} + +class AnotherShouldDispatchAfterCommitTestEvent implements ShouldDispatchAfterCommit +{ + public static $ran = false; +} + +class ShouldDispatchAfterCommitListener +{ + public function handle(object $event) + { + $event::$ran = true; + } +} + +class AnotherShouldDispatchAfterCommitListener +{ + public function handle(object $event) + { + $event::$ran = true; + } +}