search

jpfuentes2 / php-activerecord

ActiveRecord implementation for PHP
http://www.phpactiverecord.org/
Other
1.32k stars 443 forks source link

Fix type of id on newly created records #570

Open shmax opened 7 years ago

shmax commented 7 years ago

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 commented 7 years ago

@koenpunt @jpfuentes2 please review

shmax commented 7 years ago

@koenpunt @jpfuentes2 please review...

koenpunt commented 7 years ago

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

shmax commented 7 years ago

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 commented 7 years ago

@koenpunt @jpfuentes2 please review

koenpunt commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@koenpunt Are we waiting on something?