[5.7.14] Breaking change on eager loading when key is a string

[fitztrev]

• Laravel Version: 5.7.14

Description:

I just upgraded to 5.7.14 (the version released today) and there is a breaking change affecting eager loading. At least in cases where the key is a string containing a period, it converts it to an integer instead of keeping it a string like it did before.

Before, in 5.7.13 and earlier:

... WHERE foo IN ('123.456')

Now, in 5.7.14:

... WHERE foo IN (123)

I narrowed it down to the change in this method: a4405e9#diff-c8675355c360b9bd4ef485c30ff5e025R82 Changing that method back to how it was fixes it.

cc @staudenmeir

[staudenmeir]

Did you set protected $keyType = 'string'; in the model?

[fitztrev]

Ok, that fixed it. Thanks

@driesvints Just a heads up it's still a breaking change--that property hasn't been required in the past. Might be worth a mention in the changelog.

[staudenmeir]

It's mentioned in the documentation:

If your primary key is not an integer, you should set the protected $keyType property on your model to string.

[fitztrev]

I understand. But it hasn't been required in order for it to work until that commit.

[driesvints]

Okay but it was documented. Even if it somehow managed to work before it wasn't meant to work without the $keyType property being set.

[lesterchan]

Missed out $keyType as well since we use UUID. But we did set $incrementing to false. Thanks for this issue!

[rimace]

@staudenmeir @driesvints For newcomers, yes it is documented. But for those who use Laravel for years there is no mention in the upgrade guide that you to declare a keyType.
And as most people don't read the whole documentation again after an upgrade, they just don't know about it. For them, 5.7.14 introduces a breaking change.

[driesvints]

@rimace this has been in the docs for over a year at least.

[staudenmeir]

This will be changed in the next release: #26622

[rimace]

@driesvints Beside the fact that it was added to the doc about a year after the variable was introduced. What are you suggesting? Not documenting breaking changes in the upgrade guide and instead people should read the whole documentation two times a year?
It was not a breaking change in 5.7.0 or earlier, so nobody documented that in the upgrade guide, and I understand that. But this variable became mandatory in 5.7.14 and I suggest that it should be documented somewhere that it is now mandatory. But I guess it will be over with 5.7.15, lol