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

Model Hooks/Events Suggestion #557

Closed
daylightstudio opened this issue Jun 17, 2017 · 11 comments
Closed

Model Hooks/Events Suggestion #557

daylightstudio opened this issue Jun 17, 2017 · 11 comments

Comments

@daylightstudio
Copy link
Contributor

It appears that there is only one place to really hook into the saving process (onSave) when using a Model. Has there been any discussion about more hooks/events perhaps something like the following:

  • onBeforeValidate
  • onBeforeSave
  • onBeforeInsert
  • onInsert (or onAfterInsert)
  • onBeforeUpdate
  • onUpdate (or onAfterUpdate)
@lonnieezell
Copy link
Member

Some of those I'm definitely open to, and had in a base model I used to use all of the time. And the before/after insert and update were the only ones I ever used. It seems like, especially if you're using Entity's (which I realize not everyone will) you have the opportunity to make your required data changes/normalizations/etc in the Entity, and don't need the overhead of constantly firing events that aren't used a lot of the time.

Can you provide valid use cases for the others? BeforeValidate seems pointless since you'd be manipulating the data the user submitted, which typically isn't a great idea. If you did need to do that, seems like you'd do that either in the controller or, if you want to keep it in the model, override the specific method and do what needs doing prior to calling the parent.

I'm not opposed to adding events, but they need to be carefully selected because they all have a performance hit.

@daylightstudio
Copy link
Contributor Author

I'd agree that the BeforeValidate may not be super helpful since you also could have an onBeforeSave right after that, but the idea would be to make sure the values are properly cleaned and formatted for the validation.

Other use cases:

  • onBeforeSave: Can be used to set default values and format the data for saving (e.g. an array to JSON string, dates to the proper format)
  • onBeforeInsert, onBeforeUpdate: Similar to above but specific to insert only and update only. These would not be used as much as onBeforeSave but could be useful in edge cases.
  • onInsert/onAfterInsert, onAfterUpdate: Similar to onSave but specific to either Insert or Updates.

Of all the events/hooks suggested, an onBeforeSave would be the most useful I think. If implemented as a method on the model like onSave, instead of an event, the performance hit would really only be if those methods were implemented and the method check which would be pretty minimal. If implemented as an Event, I imagine that would be a little more.

@daylightstudio
Copy link
Contributor Author

BTW, I'm one of those folks that would definitely use the Entity class...

@daylightstudio
Copy link
Contributor Author

daylightstudio commented Jun 18, 2017

Another helpful hook would be an onSelect (find, findWhere, findAll... etc). This would allow a place to include things like joins necessary for every select request. I've used a base class for years in which I used that pretty heavily.

@lonnieezell
Copy link
Member

If we're not talking events here, but internal methods, I'm fine with these being added. How were you thinking of doing it? I handled it in the past like this.

@daylightstudio
Copy link
Contributor Author

That looks great. I didn't think of doing it as arrays which would give it even a little more flexibility.

@daylightstudio
Copy link
Contributor Author

Thanks for making updates for hooks. Just following up on your progress on this feature, I noticed that "onSave" still exists as an option and would be invoked differently as a method on the "Entity" model class. Is this going to be moved to the model class as an additional beforeSave and afterSave which would apply to both update and inserts? Also, is there plans for "beforeFind" which would allow there to be common joins added for selects?

Thanks again!

@lonnieezell
Copy link
Member

the onSave method might not survive. It was originally slipped in there as a bonus that would allow anyone using Entity classes to do things like save relationships, etc. This was before the official Entity class existed. Now, it's not really necessary and will probably be removed.

beforeSave and afterSave aren't needed. Callbacks can ben shared between insert and update events, so you could easily use the same method in both places:

protected $beforeInsert = ['hashPassword'];
protected $beforeUpdate = ['hashPassword'];

There are not currently any plans for a before find. I personally find setting specific joins on every single find to be potentially inefficient and rarely needed. For those times that I did, simply overload the find method in your WhateverModel class and you're golden.

@daylightstudio
Copy link
Contributor Author

daylightstudio commented Jul 10, 2017

For the beforeFind hook, joins was one example (and one that sounds like I may use more than you), but I think there are other potential use cases such as additional selects (e.g. CONCAT(first_name, ' ', last_name) AS name, restrictive where conditions based on a users permissions, etc). It's true that the method could be overwritten, but that could be true for the other hooks as well and to me it seems logical that if there is an afterFind that a beforeFind should also exist... just my 2 cents.

Also, is the trigger method suppose to have a loop of some kind to account for multiple hook methods on the model?

@lonnieezell
Copy link
Member

I guess my thinking is that these types of hooks are best used for doing stuff based on the data itself, whether it's normalizing, hashing passwords, etc, and not well suited for modifying the query itself, since the query instance isn't passed around. That doesn't mean it couldn't be used for that, but doesn't seem ideal. It seems more ideal to specify anything to do with the query in the model itself, and has the added benefit that any calls to the model would likely be more descriptive since you'd probably end up with several functions with use-specific names at that point.

I suppose we could add a beforeFind hook in, but the only data we would be able to pass in is the primary key that's being searched for. That doesn't seem very beneficial.

As for the looping - yeah, saw that last night :) Should be corrected now. I got docs written up last night. Hope to get started on actual tests tonight.

@daylightstudio
Copy link
Contributor Author

It sounds like I may use the hooks a little differently but I personally think that passing the query instance around would be really helpful to allow for further manipulation of the query (I know you can use$builder = $this->builder(); too). I like the ability to manipulate the commonly used find* method's output without the need to create additional methods if they are query parameters needed for every query.

Regardless, these hooks I think are a great addition and thanks for adding.

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

2 participants