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 afterInsert return originals? #2045

Closed
MGatner opened this issue Jun 7, 2019 · 9 comments
Closed

Model afterInsert return originals? #2045

MGatner opened this issue Jun 7, 2019 · 9 comments

Comments

@MGatner
Copy link
Member

MGatner commented Jun 7, 2019

Right now model event afterInsert returns "the original key/value pairs being updated" - which is a little ambiguous, it actually means the values you passed into insert() (including fields that didn't match allowedFields). While this info is somewhat useful, there is no way to determine what was changed, or even the original data (to perform a diff). I would like to see some way of passing either changed fields or the original fields.

@jim-parry jim-parry added this to the 4.0.0-rc.2 milestone Sep 8, 2019
@jim-parry jim-parry modified the milestones: 4.0.0-rc.2, 4.0.0-rc.3 Sep 20, 2019
@jim-parry
Copy link
Contributor

Is this a feature request or a bug report?

@MGatner
Copy link
Member Author

MGatner commented Oct 7, 2019

I think that's a question of intent. As a developer, the current behavior threw me off and I was unable to do what I expected with the event. But maybe that was my misunderstanding? It's hard to guess the scope of what model after insert events might need to do, but it makes more sense to me for the event to trigger with the inserted data rather than the submitted data.

@jim-parry
Copy link
Contributor

Makes sense.
This could be treated as a feature request (BC), or as a bug (interpretation). I am not sure how much depends on it, or how many developers are using it as is.

@MGatner
Copy link
Member Author

MGatner commented Oct 7, 2019

Hard to say! I know @lonnieezell has given Model some attention recently, if he has an opinion.

Either way, if this seems at least partway plausible I can start a PR. Think it's okay to proceed?

@jim-parry
Copy link
Contributor

If this is indeed a BC, then I think it should wait. Let's see what Lonnie says.

@lonnieezell
Copy link
Member

I think it's ok to proceed on this. When that portion of the model was originally created, there were a number of aspects with entities and the like that didn't exist yet, so things have been added on and looks like that was overlooked. But, yes, I believe it should be the data that was actually inserted.

@jim-parry
Copy link
Contributor

@MGatner Are you thinking of proceeding with this? Should it be removed from the rc.3 milestone?

@MGatner
Copy link
Member Author

MGatner commented Oct 15, 2019

It's a small change, I've been swamped but I'll get it in shortly.

@MGatner
Copy link
Member Author

MGatner commented Oct 16, 2019

Resolved with #2332

@MGatner MGatner closed this as completed Oct 16, 2019
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

3 participants