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

Events are firing before committing the transaction #8627

Closed
pdcmoreira opened this issue May 4, 2015 · 17 comments

Comments

@pdcmoreira
Copy link

commented May 4, 2015

During a transaction, if we save() the model, the 'saved' event fires event if the transaction is not commited().
I think that, if it's being saved in a transaction, it should wait for the commit() to fire the 'saved' event.
Either that, or maybe there should be a separate event for 'truly-saved' that would fire when the data is actually written in the database.

I would be happy to implement this myself, but I don't have the necessary knowlege yet.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented May 4, 2015

Hmmm, I'd say the expected behaviour is what's happening at the moment. Not sure how roll back events would work. All sounding a bit complicated, and it would be better to use some kind of higher level events.

@pdcmoreira

This comment has been minimized.

Copy link
Author

commented May 4, 2015

The thing is, I have some handlers listening for the saved() event of a model. All working fine.

Now, in a new feature I'm implementing, I must use a transaction to create 2 entries (if one fails, it rolls back). The problem is, the handlers are executed anyway even when the transaction doesn't commit.

This is a serious problem because it breaks the app's behavior, going against scalability - I must patch the event handling of the perfectly-working features with some workarounds just so they can still work with newer code.

@rayrutjes

This comment has been minimized.

Copy link

commented May 5, 2015

I personally consider that the events reflect the actions intent. You should never rely on those kind of events to implement your business logic anyway.

What if, tomorrow, you consider that your item is saved, only when it has been saved in a the database, replicated in a file, and sent by mail?

Those events have to be considered only in the context of the database and not outside. It can be useful for building some logs for example.

Rolling back is impossible or would be overly complicated. I don't see the use of that anyway.

Does my explanation make sense?

@pdcmoreira

This comment has been minimized.

Copy link
Author

commented May 5, 2015

Well, I'm launching my own custom event after the commit as a workaround.
I don't see why you say that we shouldn't rely on events to build the business logic. Events are specially helpful to maintain the SRP. The event firing method is like "this happened!" If there is a new feature that should react to this "happening", I shouldn't need to go back and refactor the firing method nor should he call a totally unrelated method from some distant class. He saves something, announces it and his job is done. Its responsibility ends there.
You can argue that the eloquent's 'saved' event is intended to fire inside a transaction even if it doesn't get committed, but I think you're wrong about the richness and reliability of events in general.

@rayrutjes

This comment has been minimized.

Copy link

commented May 5, 2015

I think that what you actually call a workaround, is your business implementation, and it is the way to go about it.

@pdcmoreira

This comment has been minimized.

Copy link
Author

commented May 5, 2015

But do you think that an event is innapropriate?

@rayrutjes

This comment has been minimized.

Copy link

commented May 5, 2015

You should definitely keep eventing in your system, don't get me wrong.
I personally let my models raise events, without firing them. And in a service layer, I dispatch the raised events of the model, only if my transaction succeeded.

Here is what looks like a command handler in a little sunday's project:

<?php

namespace App\Handlers\Commands;

use App\Commands\CloseUploadCommand;
use App\Model\DispatchesEventsTrait;
use App\Upload;
use Illuminate\Database\DatabaseManager;

class CloseUploadCommandHandler
{
    use DispatchesEventsTrait;

    /**
     * Create the command handler.
     *
     * @param DatabaseManager $db
     */
    public function __construct(DatabaseManager $db)
    {
        $this->db = $db->connection();
    }

    /**
     * @param CloseUploadCommand $command
     *
     * @return mixed
     *
     * @throws \Exception
     */
    public function handle(CloseUploadCommand $command)
    {
        $this->db->beginTransaction();
        try {
            $upload = Upload::where('hash', $command->hash())->firstOrFail();
            $upload->close();

            $this->db->commit();
        } catch (Exception $e) {
            $this->db->rollBack();
            throw $e;
        }

        $this->dispatchEventsFor($upload);

        return $upload;
    }
}

And there is the model

<?php

namespace App;

use App\Events\UploadArchiveWasBuilt;
use App\Events\UploadHasExpired;
use App\Events\UploadHasStarted;
use App\Events\UploadWasBecomingCloseToExpiration;
use App\Events\UploadWasClosed;
use App\Model\RaisesEvents;
use App\Model\RaisesEventsTrait;
use Carbon\Carbon;
use Exception;
use Illuminate\Database\Eloquent\Model;

class Upload extends Model implements RaisesEvents
{
    use RaisesEventsTrait;

    public $timestamps = false;

    protected $dates = ['expires_at'];

    protected $hidden = ['id', 'initiator_id', 'password'];

    /**
     * @param $hash
     * @param Carbon $expiresAt
     * @param User   $initiator
     * @param array  $recipients
     * @param null   $comment
     *
     * @return Upload
     */
    public static function start($hash, Carbon $expiresAt, User $initiator, array $recipients, $comment = null)
    {
        $upload = new self();
        $upload->hash = $hash;
        $upload->uploaded_at = Carbon::now();
        $upload->expires_at = $expiresAt;

        if ($comment) {
            $upload->comment = $comment;
        }

        $upload->initiator()->associate($initiator);
        $upload->save();

        $upload->recipients()->saveMany($recipients);

        $upload->raise(new UploadHasStarted($upload));

        return $upload;
    }

    /**
     * @throws Exception
     */
    public function close()
    {
        if ($this->closed == 1) {
            throw new Exception('Upload is already closed');
        }

        $this->closed = 1;
        $this->save();

        $this->raise(new UploadWasClosed($this));
    }

    /**
     * @throws Exception
     */
    public function considerCloseToExpiration()
    {
        if ($this->close_to_expiration == 1) {
            throw new Exception('Upload is already considered as close to expiration');
        }
        $this->close_to_expiration = 1;
        $this->save();

        $this->raise(new UploadWasBecomingCloseToExpiration($this));
    }

    /**
     * @throws Exception
     */
    public function erase()
    {
        if ($this->erased == 1) {
            throw new Exception('Upload was already erased');
        }
        $this->erased = 1;
        $this->save();

        $this->raise(new UploadHasExpired($this));
    }

    /**
     * @throws Exception
     */
    public function buildArchive()
    {
        if ($this->archive_ready === 1) {
            throw new Exception('Archive was already built');
        }
        $this->archive_ready = 1;
        $this->save();

        $this->raise(new UploadArchiveWasBuilt($this));
    }

    /**
     * @param $query
     *
     * @return mixed
     */
    public function scopeExpired($query)
    {
        return $query->where('expires_at', '<', Carbon::now());
    }

    /**
     * @param $query
     *
     * @return mixed
     */
    public function scopeExpiredButNotErased($query)
    {
        return $query->expired()
                     ->where('erased', 0);
    }

    /**
     * @param $query
     *
     * @return mixed
     */
    public function scopeNotErased($query)
    {
        return $query->where('erased', 0);
    }

    /**
     * @param $query
     *
     * @return mixed
     */
    public function scopeExpireSoon($query)
    {
        // 3 days
        return $query->where('close_to_expiration', 0)
                     ->where('expires_at', '<', Carbon::now()->addDay(2));
    }

    /**
     * @return bool
     */
    public function hasExpired()
    {
        return $this->expires_at->isPast();
    }

    /**
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function initiator()
    {
        return $this->belongsTo('App\User');
    }

    /**
     * @return \Illuminate\Database\Eloquent\Relations\BelongsToMany
     */
    public function recipients()
    {
        return $this->belongsToMany('App\User', 'upload_recipients');
    }

    /**
     * @return \Illuminate\Database\Eloquent\Relations\HasMany
     */
    public function files()
    {
        return $this->hasMany('App\File');
    }

    /**
     * @return string
     */
    public function getArchiveStoragePath()
    {
        return 'archives/'.$this->hash;
    }

    /**
     * @return bool
     */
    public function archiveIsReady()
    {
        return $this->archive_ready == 1;
    }
}

I found this approach to be expressive and maintainable.
Events are only fired if the whole transaction succeeded.

@pdcmoreira

This comment has been minimized.

Copy link
Author

commented May 6, 2015

That's not too different from what I have. I'm closing this for now, I'm not sure if the issue is expected behavior or not.

@pdcmoreira pdcmoreira closed this May 6, 2015
@rayrutjes

This comment has been minimized.

Copy link

commented May 6, 2015

If it this is close from what you have you don't need those database events.

@pdcmoreira

This comment has been minimized.

Copy link
Author

commented May 6, 2015

I just wanted to have them at the model level.

@rayrutjes

This comment has been minimized.

Copy link

commented May 6, 2015

In my example, events are raised in the model where they belong, you are totally right.

@fntneves

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

@pdcmoreira I had the same issue. Built this package to tackle this issue: https://github.com/fntneves/laravel-transactional-events

P.S.: Sorry for posting on an old issue, but I faced the same issue.

@judgej

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

I think the outcome of this should be an update to the documentation, stating that model observers should NOT be used or relied upon if transactions are going to be used in the application. I do not accept this should be expected behaviour, but since it is the actual behaviour, then the documentation needs to be clear.

@flyingL123

This comment has been minimized.

Copy link

commented Jan 23, 2019

I agree. I just ran into this same issue, and it was definitely not the behavior I was expecting. In my application I save an Order within a transaction, and then save some additional data about the order to another table within the same transaction. Saving to the other table may throw an exception since it has unique constraints on it.

In that case, the transaction prevents the order from being written to the database, which is what I want.

The order also makes use of the 'created' event to trigger a listener that exports the email address on the order to Mailchimp. The listener is getting triggered even if the transaction is rolled back, which obviously triggers an error because the order doesn't actually exist in the database.

The Illuminate\Queue\SerializesAndRestoresModelIdentifiers ends up throwing a ModelNotFoundException.

@mvanduijker

This comment has been minimized.

Copy link

commented Feb 13, 2019

Would be adding additional" transactional" events be a solution? Rails does it like this https://guides.rubyonrails.org/active_record_callbacks.html#transaction-callbacks

@mvanduijker

This comment has been minimized.

Copy link

commented Feb 13, 2019

Ah there already in issue in laravel/ideas#1441

@judgej

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I'm not sure a single after-commit event is being sought here. It is more a case of being able to delay some of the events triggered until after the next commit, or discard them after the next rollback. It would be kind of like queuing them all up, then running through them or discarding the queue in one go at the end.

Some events may not need to be delayed, if those events just result in further database updates in the same session (that would be a development choice).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.