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

Transaction issue inside the eloquent observer saved event #1759

Closed
lk77 opened this issue Jul 19, 2019 · 6 comments
Closed

Transaction issue inside the eloquent observer saved event #1759

lk77 opened this issue Jul 19, 2019 · 6 comments

Comments

@lk77
Copy link

lk77 commented Jul 19, 2019

Description:

Hello,

we have an issue with nova inside eloquent observers saved events,
we expect that any transaction that nova opens, should be closed when the saved observer method is called, which is not the case.

We want to do something in the observer, like a file deletion, an http request or an event broadcast, something that can be undone. And the fact that transaction is still open and that the row doesn't exists in database yet is not consistent.

That action in the observer will cause other actions in cascade of third parties, like incoming requests. So a incoming request, that wants to recover a newly created instance, will get an error, because row doesn't exist yet (transaction is still going). It depends on the latency between the server and the client, but it's not quite right.

Steps To Reproduce:

1 / git clone https://github.com/lk77/nova-observer-transaction-bug
2 / composer update
3 / cp .env.example .env // change database informations
4 / php artisan serve
5 / go to http://localhost:8000/nova/resources/tests
6 / click on the create button
7 / open F12 network on chrome (or whatever you are using)
8 / submit a new test
9 / inside http://localhost:8000/nova-api/tests request, see the dump saying :

"transation status : open"

the code responsible for the dump inside App\Observers\TestObserver::saved :

dd('transation status : ' . (\DB::transactionLevel() > 0 ? 'open' : 'closed'));

10 / see in database the test row has not been added.

For now, the solution seems to add this inside resource :

    /**
     * Return the location to redirect the user after creation.
     *
     * @param \Laravel\Nova\Http\Requests\NovaRequest $request
     * @param \App\Nova\Resource $resource
     * @return string
     */
    public static function redirectAfterCreate(NovaRequest $request, $resource)
    {
        // do custom process here, after transaction

        return parent::redirectAfterCreate($request,  $resource);
    }

wich is more like a hack.

A possible solution will be to introduce a hook inside resource, called after transaction in controller.

thanks.

@lk77 lk77 changed the title Transaction issue inside nova observer saved event Transaction issue inside the eloquent observer saved event Jul 19, 2019
@meyer59
Copy link

meyer59 commented Jul 19, 2019

We have the same problem here.
Wen we reach the saved Observer method, the model is not actually saved in the db.
How we could lunch an action after the transaction commit ?

@bonzai
Copy link

bonzai commented Jul 19, 2019

Can you install fntneves/laravel-transactional-events to see if it fixes your issue?

@lk77
Copy link
Author

lk77 commented Jul 22, 2019

It does not fix my issue,
this package is for custom events,
not the eloquent events, and is not working with observers.

@bonzai
Copy link

bonzai commented Jul 22, 2019

@lk77: It's working fine for me. Are you sure you are using this package correctly? If you are using observers, you have to update the config because it will ignore all the Eloquent events.

Also, for the record, this issue isn't related to Nova, but Laravel - laravel/framework#8627.

@lk77
Copy link
Author

lk77 commented Jul 23, 2019

yes, i have commented all the lines in the exclude array, and the transaction is still open inside the saved method of the observer.

I need to add eloquent.saved to the transactional array aswell for this to work

We resolved this, by moving our code inside the redirectAfterCreate and redirectAfterUpdate methods inside resource and it is working fine. We do not want to rely on a third party package, it's messing a lot with our tests, and having to override dispatcher does not seems to be a good solution.

@mfn
Copy link

mfn commented Jun 17, 2020

Hello everyone,

based on an existing package and with permission of its original author, I've drafted a possible solution to tackle the "transactional events" problem.

At the moment it's a draft PR in my Laravel forked repo to first gather feedback from the community before approaching to create an official PR, to give things time to unfold / develop.

I invite everyone to participate in mfn/laravel-framework#1 and share your feedback.

Thank you!

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

No branches or pull requests

4 participants