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

[5.3] Adds additional check of keyType to avoid using an invalid type for eager load constraints #16452

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

bryanashley
Copy link

Original PR was made against 5.2, see here

@bryanashley
Copy link
Author

bryanashley commented Nov 17, 2016

@GrahamCampbell Made it against 5.3

@bryanashley bryanashley changed the title Adds additional check of keyType to avoid using an invalid type for eager load constraints [5.3] Adds additional check of keyType to avoid using an invalid type for eager load constraints Nov 17, 2016
@taylorotwell
Copy link
Member

How do you have an incrementing primary key that is not a integer? Do some databases support that?

@bryanashley
Copy link
Author

bryanashley commented Nov 18, 2016

@taylorotwell Its not an exactly an incrementing primary key, but we'd like to use incrementing true because we are using UUIDs that are set by psql. Incrementing true is what tells eloquent to set the id from the database after insertion. It's similar to the issue discussed in this PR. Quote from that PR that explains the issue more eloquently:

Our codebase uses string uuids generated at the database layer (postgres) rather than the application layer, so technically we needed $incrementing to be set to true (since the database was handling "incrementing" in our case), however our ID type was string rather than int.

@bryanashley
Copy link
Author

bryanashley commented Nov 18, 2016

I think that incrementing might not be the best name for that property, given it is only used in this check, and the check that determines whether or not to use the database to set the model's id.

The two locations incrementing is currently used are:

  • When eager loading a belongsTo relationship where the keys are null, if incrementing is false, use null instead of 0 for model keys (seen here)
  • When inserting a new row into the database, if incrementing is false, do not use the database to set the id on the new row (seen here)

Something like generated might make more sense to describe a primary key that is set by your database.

Either way, in order to do this in a non-breaking manner, this seemed like the ideal solution.

@taylorotwell taylorotwell merged commit c803376 into laravel:5.3 Nov 21, 2016
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