php-activerecord icon indicating copy to clipboard operation
php-activerecord copied to clipboard

Fix type of id on newly created records

Open shmax opened this issue 7 years ago • 8 comments

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 avatar Apr 20 '17 16:04 shmax

@koenpunt @jpfuentes2 please review

shmax avatar Apr 20 '17 17:04 shmax

@koenpunt @jpfuentes2 please review...

shmax avatar May 06 '17 02:05 shmax

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

koenpunt avatar May 06 '17 19:05 koenpunt

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 avatar May 07 '17 01:05 shmax

@koenpunt @jpfuentes2 please review

shmax avatar May 08 '17 16:05 shmax

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.

koenpunt avatar May 08 '17 17:05 koenpunt

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 avatar May 08 '17 19:05 shmax

@koenpunt Are we waiting on something?

shmax avatar May 16 '17 17:05 shmax