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

Fix type of id on newly created records #570

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shmax
Copy link
Contributor

@shmax shmax commented Apr 20, 2017

When new-ing a new record and doing a save, the primary key is stored as a string. It's not until you do a find that it is properly converted to the expected integer. This is because the id is retrieved directly from static::connection()->insert_id whereas the other attributes are passed through set_attributes_via_mass_assignment, which does a cast on each of the values.

Fix is to do a similar cast on the the id before it is stashed in attributes during the insert step.

@shmax shmax changed the title Fix id cast Fix type of id on newly created records Apr 20, 2017
@shmax
Copy link
Contributor Author

shmax commented Apr 20, 2017

@koenpunt @jpfuentes2 please review

@shmax
Copy link
Contributor Author

shmax commented May 6, 2017

@koenpunt @jpfuentes2 please review...

@koenpunt
Copy link
Collaborator

koenpunt commented May 6, 2017

What problem does this solve? Also please add a test that clarifies the problem you're experiencing.

@shmax
Copy link
Contributor Author

shmax commented May 7, 2017

I did add a test. Did you look at the changes? The problem is that the id is not necessarily of the correct type when creating a new record--it's always a string, even in cases where it should rightly be an integer. I explained this in my original comment; how can I make it more clear for you?

@shmax
Copy link
Contributor Author

shmax commented May 8, 2017

@koenpunt @jpfuentes2 please review

@koenpunt
Copy link
Collaborator

koenpunt commented May 8, 2017

Like I said before, what problem does it solve, apart from the strict comparison? Also bugging maintainers like this most of the time results in the adverse effect.

@shmax
Copy link
Contributor Author

shmax commented May 8, 2017

Well, my argument is that type matters. This library seems to implicitly agree with me, as it makes a point of casting type when doing a find. Is your argument really that it's okay for the id to be of one type on create, and a different type after a find? Can you explain why you don't think this is a problem?

Do you need me to demonstrate to you with a real world use case why this is a problem? Okay, I can do that:

On my site I cache records (using the caching feature that I myself added to this library). I also have a feature that allows users to edit records using a form. I need to be careful about cases where two users are editing the same record at the same time--I don't want either of them to stomp on the other's changes and lose work. So, when a user begins editing a record I serialize the current state of his record, store it in the form as a hidden input, then when he submits the form I repeat the serialization step on the current state of the record (as taken from the database, which in my case comes from cache), and see if the result is different from what I stashed in the hidden input field. If they are, I know a change happened from some other user, and I can show a warning. See where this is going? Due to your bug, I can get a different result for the serialization step even for a newly-created record that has not yet received any changes. This is a long-standing bug that has been frustrating my users off and on for years, and I was only recently able to track it down to this issue in this library.

And I apologize for pestering, but I don't want to wind up in the same limbo as the other 130 issues and 70 PRs currently being ignored. Tell me what I can do to expedite this.

@shmax
Copy link
Contributor Author

shmax commented May 16, 2017

@koenpunt Are we waiting on something?

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.

2 participants