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

[5.5] Add 'retrieved' model event #20852

Merged
merged 3 commits into from
Sep 2, 2017
Merged

[5.5] Add 'retrieved' model event #20852

merged 3 commits into from
Sep 2, 2017

Conversation

miclf
Copy link
Contributor

@miclf miclf commented Aug 30, 2017

As discussed with @taylorotwell at Laracon EU, this simple PR allows Eloquent models to fire a retrieved event as soon as their contents are retrieved from the database.

Use case

This comes from the following use case:

  1. I need to do some manipulations with the attributes of a model as soon as this model is retrieved from the database.
  2. I also need to do the opposite operation just before the model is saved back to the DB.

The second case is pretty easy: I just need to register an event on my model, for example in its boot() method:

// In MyModel.php

protected static function boot()
{
    parent::boot();

    static::saving(function ($model) {
        // Do some stuff...
    });
}

However, there is no symmetric way to execute code when an existing model is retrieved from the database.

The currently most straightforward way to implement this behavior is to extend the newFromBuilder() method on our custom model, like so:

// In MyModel.php

public function newFromBuilder($attributes = [], $connection = null)
{
    // First, retrieve the model using the original method.
    $model = parent::newFromBuilder($attributes, $connection);

    // Then, we do our custom stuff.
    // ...

    return $model;
}

This works, but it’s clearly not as elegant nor as practical as just registering a listener on the model.

What this PR makes possible

This pull request would allow to satisfy the use case, for example, in the following way:

// In MyModel.php

protected static function boot()
{
    parent::boot();

    static::retrieved(function ($model) {
        // Do some stuff...
    });

    static::saving(function ($model) {
        // Do some other stuff...
    });
}

Of course, you could also use an observer class instead, or any other working way you could think of.

Notes

  • I don’t propose any retrieving event, because I cannot find any use case for it. More importantly, I don’t really see any place where a retrieving event would make sense. Just before hydrating the model? Before running the SQL query? I don’t have any satisfying answer;
  • Since none of the *ed model events have tests (only the *ing events require tests), I didn’t add any;
  • If this PR is merged, I’ll do another one to update the docs.

@GrahamCampbell
Copy link
Member

Thanks for the PR. Good work. 👍

My only concern is, are other people already using the retrieved method for something else, and if so, this change is technically breaking.

@decadence
Copy link
Contributor

Great. Missed that.

@miclf
Copy link
Contributor Author

miclf commented Aug 30, 2017

@GrahamCampbell Correct, but this would be the case whatever the name of the event is. Do you see any way to go around this potential issue? If not, this change could be added to the 5.5 upgrade guide (I guess now is the best time to do so).

@tillkruss Do you think I should also update the 5.5 changelog?

@tillkruss
Copy link
Contributor

You don't have to, I'll already do it anyhow.

@j-shelfwood
Copy link

Loving this PR! A while back I had a great usecase for this, I will definately go back and refactor if this is merged in.

@taylorotwell taylorotwell merged commit bc9ae14 into laravel:5.5 Sep 2, 2017
@taylorotwell
Copy link
Member

Would be worth having a documentation PR for this.

@miclf
Copy link
Contributor Author

miclf commented Sep 2, 2017

@taylorotwell Here is the PR for the docs: laravel/docs#3606

Thanks for the merge :)

@j-shelfwood
Copy link

Nice to have this handled, looking forward to using it!

@Kyslik
Copy link
Contributor

Kyslik commented Jul 20, 2018

Aren't we firing this in the wrong place? Eager loaded relationships are not available in retrieved event.

protected static function boot()
{
    parent::boot();

    static::retrieved(function (SomeModel $m) {
        dd($m); // no relationships were hydrated at this point
    });
}

...

SomeModel::with('whateverRelation')->get();

ping @miclf

@miclf
Copy link
Contributor Author

miclf commented Jul 20, 2018

@Kyslik This event is about when the model itself is retrieved, independently of any relationship it might have, so the current implementation makes sense to me.

I coded this almost a year ago, so I obviously can’t retrieve all of the details off the top of my head, but I think that firing this event as soon as technically possible gives more possibilities to the developer to potentially alter the following operations that are done with the model (including, but not limited to, how and which relationships are (eager-)loaded.

There might also be constraints preventing to call this event later in the retrieval process (resulting in it to be called ‘too late’), but I’m just speculating here.
I’m also wondering how this would behave if the eager-loaded models also make use of the retrieved event.

If you feel and think this event should be triggered after the retrieval of relationships, do feel free to submit a change. I cannot guarantee I would have time to help or look into it, but I’ll certainly do it if I can :)

Another event would also be a possible implementation (something like relationships-retrieved), so developers would have different choices for different use cases. But this may also start to look a bit gross, I don’t know.

@Kyslik
Copy link
Contributor

Kyslik commented Jul 20, 2018

@miclf thank you for taking time to respond so fast.

I actually did some magic with relations in retrieved event to avoid n+1 problem and then to my surprise I made it even worse 🤓 🌴 (facepalm), thats why I went to hunt what is going on and found this PR.

To me retrieved should fire after everything I requested is there to work with (including relationships), obviously your explanation makes sense as well.

There were talks about some more eventing in laravel/ideas#1197


I took a look at the code and I don't think it is possible to fire event in model itself after relationships are eager-loaded. I bet I do not understand it as much...

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.

7 participants