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.7] Prevent breaking eager loading with string keys #26622

Merged
merged 1 commit into from Nov 26, 2018

Conversation

Projects
None yet
5 participants
@staudenmeir
Contributor

staudenmeir commented Nov 26, 2018

The eager loading improvement introduced in #26434 and #26453 broke some applications with string keys.

The documentation advices users with string keys to set both $incrementing = false and $keyType = 'string'. However, the affected users only adjusted $incrementing and left the default $keyType = 'int' unchanged. This has been working fine up to the latest release, as the $keyType is only used for auto-incrementing keys.

With this PR, the performance improvement is only applied to auto-incrementing primary keys. As a result, users with non-incrementing integer keys don't benefit from it anymore (but there probably aren't that many cases).

@stayallive

This comment has been minimized.

Contributor

stayallive commented Nov 26, 2018

So this broke my app 💯 never noticed that $keyType should be set... whoops!

Thanks for finding this!

@taylorotwell taylorotwell merged commit 231fda5 into laravel:5.7 Nov 26, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@staudenmeir staudenmeir deleted the staudenmeir:eager-loading-string branch Nov 26, 2018

@aszaffranietz aszaffranietz referenced this pull request Nov 28, 2018

Closed

compileWhereInRaw #1649

aszaffranietz added a commit to deinhandy/laravel-mongodb that referenced this pull request Nov 28, 2018

@vlakoff

This comment has been minimized.

Contributor

vlakoff commented Dec 1, 2018

By any chance, can you think of a way to get back the optimization with non-incrementing integer keys?

@staudenmeir

This comment has been minimized.

Contributor

staudenmeir commented Dec 2, 2018

No, not without some kind of flag that users would set in their models. So basically $keyType but "for real"...

You can apply the optimization to non-incrementing integer keys by extending the relationships and overriding whereInMethod(). But that's probably only worth the effort if you really benefit from raw queries.

@miklcct

This comment has been minimized.

Contributor

miklcct commented Dec 4, 2018

I think you should revert to the 5.7.14 behaviour in 5.8 for non-incrementing integer keys, and mention in the upgrade guide as a "breaking change", i.e. in 5.8 all integer keys should get the performance improvement and mention in the upgrade guide that the user must set $keyType for non-integer keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment