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

[8.x] Delay dispatching listeners until transaction commits #35434

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Dec 1, 2020

This PR is similar to #35422. It delays running listeners until transactions commit.

DB::transaction(function(){
    $user = User::create(...);

   //...
});

Having a listener to the UserCreated event:

class SendWelcomeEmail{
    public $disptachAfterCommit = true;

    public function handle(){
        // ...
    }
}

That listener will not run until after the transaction commits. If the transaction was rolledback, the listener will be discarded.

@crnkovic
Copy link
Contributor

crnkovic commented Dec 1, 2020

What if we added an interface like ShouldWaitForCommit / WaitsForTransactionCommit instead of using properties? Can be also added to jobs.

@kafkiansky
Copy link

Very strange implementation. Why the event should know when it will be dispatch? Event is just a message, it must be decoupled from environment and has no affect to dispatch process. If you want to dispatch the event after transaction is commited, use something like dispatchAfterCommit(). And implementation will be simple even for laravel < 8.0:

public function dispatchAfterCommit(object $event, string $target = TransactionCommitted::class): void
    {
        $this->listen($target, function () use ($event): void {
            $this->dispatch($event);
        });
    }

Such implementation allow us to dispatch the event with different way in different situation. With your PR if i want to dispatch before transaction is commited i need to create the event object, change his property and after that dispatch it.

@themsaid
Copy link
Member Author

themsaid commented Dec 7, 2020

@kafkiansky this has nothing to do with events but rather listeners.

@kafkiansky
Copy link

kafkiansky commented Dec 7, 2020

@themsaid, but... you defined a property inside the event

@themsaid
Copy link
Member Author

themsaid commented Dec 7, 2020

@kafkiansky no it's on the listener.

@kafkiansky
Copy link

kafkiansky commented Dec 7, 2020

@themsaid, my mistake, sorry, i have overlooked. but does not matter, it's even worse because you has no way to change this property if need.

@LastDragon-ru
Copy link
Contributor

@kafkiansky, as you can see PR uses $listener = $this->container->make($class); so you can set this property inside the constructor or inject configuration inside and use it to set this property.

@kafkiansky
Copy link

@LastDragon-ru, what problem it solve? This PR add more collisions because in cases when listener implements ShouldQueue interface dispatcher use reflection->newInstanceWithoutConstructor() to create a listener and the properties values in this case not be visible in the constructor. i think, that the properties is not a best way to control code flow.

@LastDragon-ru
Copy link
Contributor

LastDragon-ru commented Dec 7, 2020

what problem it solve?

Please see original #35373 issue, if short: sometimes often code must be run only after commit, and this

class SendWelcomeEmail{
    public $disptachAfterCommit = true;

    public function handle(){
        // ...
    }
}

much better than:

class SendWelcomeEmail{
    public function handle(){
        DB::afterCommit(function (){
            Mail::send(...);
        });
    }
}

ShouldQueue interface dispatcher use reflection->newInstanceWithoutConstructor() to create a listener and the properties values in this case not be visible in the constructor

Yep, IMO Laravel should use Container instead of newInstanceWithoutConstructor. There are few (closed) issues to change this odd behavior, and probably I will create a new one when this PR will be merged)

@kafkiansky
Copy link

@LastDragon-ru,

There are few (closed) issues to change this odd behavior

Yep, they was mine. And i still confuse why the PR was rejected.

I agree, but Laravel always used properties instead of interfaces

I think, interfaces in this case also is bad idea and even worse than properties. Interface as marker looks bad, sorry. Just use method, why not? Method add more manageable and obvious behavior.

@crnkovic
Copy link
Contributor

crnkovic commented Dec 8, 2020

@kafkiansky

I think, interfaces in this case also is bad idea and even worse than properties. Interface as marker looks bad, sorry.

ShouldQueue, ShouldBeUnique, ShouldBroadcast, ShouldBeUniqueUntilProcessing, ShouldBroadcastNow

@kafkiansky
Copy link

@crnkovic, and what? interfaces in this case make dispatch management more complicated. imagine that you have an listener that trigger when user registration event occur and you have many cases when registration is complete: an api, an web with order issued and without. registration may be too complicated and require transaction, but may be easy and without transaction, but however the event always needs to be dispatch. how you plan change listener behavior? you don't have access to the listener property on runtime.

final class RegistrationOccur
{
}

final class RegistrationOccurListener
{
    public $disptachAfterCommit = true;
    public function when(RegistrationOccur $event) {}
}

Complicated registration:

$this->connection->transaction(function () {
  $user = User::createFromWeb(...);
  $order = Order::issue(...);
  // other things

 $this->dispatcher->dispatch(new RegistrationOccur($user->id));
}); // yeah, property `$disptachAfterCommit` save us from transaction failure! 

Easy registration:

$user = User::createFromApi(...);

$this->dispatcher->dispatch(new RegistrationOccur($user->id)); // hm, where my event??? i lost them, analysts will kill me((

But what about O from SOLID and what about... tests? I have changed value of property $disptachAfterCommit and need to change code in tests, add transaction in a place where it has never been and it is not needed there. This PR adds complexity where it is not needed. As I said earlier, just use the method:

public function dispatchAfter(object $event, string $target = TransactionCommitted::class): void
{
     $this->listen($target, function () use ($event): void {
          $this->dispatch($event); // Or other implementation here.
     });
}

This method allow to dispatch not after TransactionCommitted event but after any other and add more manageable behavior.

@crnkovic
Copy link
Contributor

crnkovic commented Dec 8, 2020

If you don't have any transactions, listener will still be dispatched. I agree with the method though. We have a shouldQueue method on the listeners. shouldDispatchAfterTransactionCommits is a good idea also :)

@LastDragon-ru
Copy link
Contributor

LastDragon-ru commented Dec 8, 2020

you don't have access to the listener property on runtime.

Actually, the Dispatcher check and call if present shouldQueue() so you can use it if really needed (or you can even override Dispatcher (as I doing))

(obviously, it work only for the queued listeners)

@kafkiansky
Copy link

kafkiansky commented Dec 8, 2020

@LastDragon-ru, how you plan to manipulate with listener's method if you don't have access to the listener within dispatch process? conditions to run within transactions or not may be too complicated and depend on code where event is dispatch but not of listener. May be you have code for this cases?

@LastDragon-ru
Copy link
Contributor

how you plan to manipulate with listener's method if you don't have access to the listener within dispatch process?

I don't think that this is the right behavior - in most cases, listeners must be dispatched only if committed. For example: if $user = User::createFromApi(...); failed the RegistrationOccur must not be dispatched because $user->id is null and thus listener will fail when handle() will be called. In case when you need dispatch regardless of the result of User::createFromApi(...); (and/or transaction) you can add a second listener without $disptachAfterCommit.

@kafkiansky
Copy link

kafkiansky commented Dec 8, 2020

@LastDragon-ru, and i think so: listeners always should be dispatched only if transaction commited, but however for this need individual method because it make behavior more explicit. the listener should not know about such details as transaction life cycle.

you can add a second listener without $disptachAfterCommit

No, is boilerplate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants