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

[7.x] Cast primary key to string when $keyType is string #33930

Merged
merged 3 commits into from Aug 19, 2020
Merged

[7.x] Cast primary key to string when $keyType is string #33930

merged 3 commits into from Aug 19, 2020

Conversation

stevethomas
Copy link
Contributor

@stevethomas stevethomas commented Aug 19, 2020

Problem

Given a model with a string primary key:

 public $incrementing = false;

 protected $keyType = 'string';

and then running this on the model:

PaymentProvider::find(0);

I expect a null result, but in fact I get the first row in the table.

Looking at the underlying query, sure enough it runs select * from payment_providers where id = 0; MySQL seems to have a quirk where it will return all rows when comparing a string to a 0 integer.

MySQL aside, I think Laravel should take care of casting the primary key according to $keyType to prevent unexpected results.

Solution

This PR checks the primary key type, and casts to a string when required in whereKey() and whereNotKey().

Caveats

PaymentProvider::find([0]);

This query will return a collection of all rows in the table. I wasn't sure how to deal with an array of IDs, so I dealt with the singular lookup only.

@tobiasthaden
Copy link
Contributor

I'm not sure if casting the attribute is the best solution. You specified the $keyType should be a string but you provide an integer. So the framework should may throw an InvalidArgumentException.

@taylorotwell taylorotwell merged commit 56d2fec into laravel:7.x Aug 19, 2020
driesvints pushed a commit that referenced this pull request Aug 27, 2020
* Cast primary key to string when $keyType is string

* fix test

* fix remaining tests
@driesvints
Copy link
Member

@stevethomas FYI: this broke passing null as input: #34029

This should also have been sent to 6.x LTS (see our contribution guide). I've ported this to 6.x with an additional fix for the bug which will also make its way to 7.x: #34031

taylorotwell pushed a commit that referenced this pull request Aug 27, 2020
* [7.x] Cast primary key to string when $keyType is string (#33930)

* Cast primary key to string when $keyType is string

* fix test

* fix remaining tests

* Fix bug with whereKey null

Co-authored-by: Steve Thomas <steve@codinglabs.io>
@stevethomas
Copy link
Contributor Author

Noted @driesvints, i'll be sure to PR to both branches in the future.

@driesvints
Copy link
Member

@stevethomas you don't need to PR to both. We merge 6.x into 7.x and 7.x into master regularly.

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.

None yet

4 participants