Factory create() doesn't respect the model $keyType

[Celant]

• Laravel Version: 9.41.0
• PHP Version: 8.1.13
• Database Driver & Version: MySQL 8.0 (sail)

Description:

All of my models extend from a BaseModel class, which sets $incrementing = false and $keyType = 'string'. It also sets up a saving event on each model that will populate the primary key (if null) with a new Snowflake ID int.

This has worked fine so far, and if I do a WhateverModel::all() I get a collection returned, and the primary key of each model is correctly casted to a string. Prior to 9.x, I believe this also worked if I did a WhateverModel::factory()->create(). I would get back a model with a properly casted primary key.

As of testing in 9.x, the Factory create() function doesn't respect the $keyType defined on the model. Instead of it casting to a string, it is returned as an int. The database row and the $keyType are both set to be a string, and although the model saving event does initially submit the snowflake as an int, up until 9.x this has not seemed to matter as it would still be properly cast back.

Steps To Reproduce:

1. Make a model with $incrementing = false and $keyType = 'string' set.
2. Migration should set the key column type to a string
3. From within the model's saving event, set the key to an integer value
4. Create a new model using its factory and store the result
5. Analyse the result to see that the key attribute is still an integer

[github-actions[bot]]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[abbood]

Hey @Celant, can you please create a repo that does your reproduction steps? I will be happy to dig in further if you do.

[Celant]

Thanks @abbood! I've created Celant/laravel-bug-reproduction to help. It's only got two commits which is the base new project, and then the changes I made to reproduce the issue, which hopefully makes it easier to see exactly what I changed!

It has a single unit test (FactoryCastModelKeyTest) which demonstrates the issue

[abbood]

Thanks @Celant will take a look!

[abbood]

Thanks @Celant for the repo. The commit says that it's the bare minimum to repro the issue. Does the bug happen even without using snowflake? Because now I'm wondering if this is a Laravel problem or a snowflake problem

[Celant]

Good call. I've added another commit which removes that package, and replaces it with some code that achieves a similar affect (sets the model key to a random integer on saving, if it was null). Tests are still failing with that change it seems!

[abbood]

Excellent! Now we can debug with a clear conscience 😂

[abbood]

@Celant You mentioned something that made me curious:

This has worked fine so far, and if I do a WhateverModel::all() I get a collection returned, and the primary key of each model is correctly casted to a string. Prior to 9.x, I believe this also worked if I did a WhateverModel::factory()->create(). I would get back a model with a properly casted primary key.

So like a good Samaritan I went ahead and created a laravel 8 repo and plugged your code in it. I ran the test and got this error:

./vendor/bin/phpunit tests/Unit/FactoryCastModelKeyTest.php
PHPUnit 9.5.27 by Sebastian Bergmann and contributors.

EE                                                                  2 / 2 (100%)

Time: 00:00.236, Memory: 26.00 MB

There were 2 errors:

1) Tests\Unit\FactoryCastModelKeyTest::testFactoryCreate
Error: Call to undefined function Database\Factories\fake()

/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/database/factories/TestModelFactory.php:20
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:424
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:403
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:387
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/GuardsAttributes.php:157
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:392
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:360
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:258
/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/tests/Unit/FactoryCastModelKeyTest.php:16
phpvfscomposer:///Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/phpunit/phpunit/phpunit:97

I went back to your code and saw that in this commit you added this line

'test_column' => fake()->sentence(),

which simply doesn't work in laravel 8

[abbood]

@Celant I confirmed that this behavior also existed in laravel 8. I created a repo with minor irrelevant changes to make sure it works on laravel 8, and the unit test returns the same thing:

./vendor/bin/phpunit tests/Unit/FactoryCastModelKeyTest.php
PHPUnit 9.5.27 by Sebastian Bergmann and contributors.

F.                                                                  2 / 2 (100%)

Time: 00:00.249, Memory: 26.00 MB

There was 1 failure:

1) Tests\Unit\FactoryCastModelKeyTest::testFactoryCreate
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'string'
+'integer'

/Users/abdullahbakhach/dev/sandbox/laravel-8-repro/tests/Unit/FactoryCastModelKeyTest.php:18
phpvfscomposer:///Users/abdullahbakhach/dev/sandbox/laravel-8-repro/vendor/phpunit/phpunit/phpunit:97

FAILURES!
Tests: 2, Assertions: 2, Failures: 1.

[abbood]

@Celant the fact is that the two ways of retrieving the TestModel are very different. In the first case you created it on the fly then read it from memory:

        $test = TestModel::factory()->create();

but in the second case you retrieved it from the database:

        TestModel::factory()->create();
        $test = TestModel::all()->first();

I guess you can argue that this is a bug with laravel, but I doubt that this is a regression bug

[hypnodev]

@Celant I don't think it's a bug, because you are adding logic to the saving event, then Laravel will not attend within it and this is why also when you are using first it's working.

So, inside your saving closure add a casting to string (string)$id and this will works! :)

Anyway, I suggest you to use creating instead of saving otherwise your ID will be generated every time you save the model and I don't think it's the right behaviour (at least from the example)

[driesvints]

Seems most can agree this isn't a bug.