Events are firing before committing the transaction

[pdcmoreira]

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]

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]

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]

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]

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]

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

[pdcmoreira]

But do you think that an event is innapropriate?

[rayrutjes]

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]

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.

[rayrutjes]

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

[pdcmoreira]

I just wanted to have them at the model level.

[rayrutjes]

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

[fntneves]

@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]

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.