[11.x] Fix foreignIdFor() when the foreign key is a non-incrementing integer other than ULID #53696
Conversation
edgrosvenor
changed the title
edgrosvenor
changed the title
| @@ -405,6 +405,22 @@ public function testGenerateRelationshipColumnWithIncrementalModel() | |||
| ], $blueprint->toSql($connection, new MySqlGrammar)); | |||
| } | |||
|
|
|||
| public function testGenerateRelationshipColumnWithNonIncrementalModel() | |||
| { | |||
| require_once __DIR__.'/stubs/EloquentModelNonIncrementedIntStub.php'; | |||
There was a problem hiding this comment.
Please follow existing autoloading structure when importing stub class
There was a problem hiding this comment.
I'd be happy to, but I'm not totally sure what you mean. Visually, this looks exactly like what you did in testGenerateRelationshipColumnWithUuidModel(), so a little guidance would be greatly appreciated.
There was a problem hiding this comment.
As far as I can tell, this is the way stubs are already being imported in this test.
See lines 426, 443, 513, 546...
There was a problem hiding this comment.
Fixed, stubs are for mocking and fixtures are test code/data used in tests
There was a problem hiding this comment.
Thanks. I was just going by what the code surrounding my work was doing. I haven't been paying close attention to how coding standards have evolved there. This is actually much more intuitive!
Signed-off-by: Mior Muhammad Zaki <[email protected]>
This issue has been raised before: #46211
When using a non-incrementing integer as a primary key and then referencing that key using
foreignIdFor()the resulting column is a UUID, which is the default return on the method. We caught this because we use snowflakes as IDs in an app we're working on and when we moved from sqlite to postgres, we started getting errors (sqlite is too forgiving sometimes).If I were doing this on the 12.x branch, I might be inclined to use a check similar to the
HasUlids::classcheck forHasUuids::classand make a non-incrementing big integer the default instead. I think most people who are using snowflakes as IDs are using https://github.com/glhd/bits which offers aHasSnowflakestrait similar to the ones provided for UUIDs and ULIDs in Laravel, but since it's a 3rd party package, checking for that trait here doesn't make sense. But it feels like changing the default return value is too big a change to make outside of a major release, even though it is extremely unlikely that it would cause anyone any problems but might correct an unseen error for people using SQLite.I've simply added another check under the ULID check for non-incrementing integers and left the rest of the method as-is. If a more aggressive fix is desirable either on this branch or on 12.x I'd be happy to take it on.