[5.2] Adds idType to model in order to prevent assumptions about ids

[stevenleeg]

Hi there,

We recently upgraded to Laravel 5.2 and noticed some weird behavior when trying to save models and tracked it down to an incorrect assumption made in a recent change to \Illuminate\Database\Model, namely in the getCasts() function:

    /**
     * Get the casts array.
     *
     * @return array
     */
    public function getCasts()
    {
        if ($this->getIncrementing()) {
            return array_merge([
                $this->getKeyName() => 'int', // This was our culprit
            ], $this->casts);
        }

        return $this->casts;
    }

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.

As an interim solution we just added the following to a BaseModel class we created which all of our models inherit from:

public function getCasts()
{
    return $this->casts;
}

this just overrides Laravel's getCasts function to get rid of the int typecast on the primary key. It worked but felt kind of jank, and we're sure we're not the only ones who have this use case. This PR proposes a solution which keeps Laravel's defaults, but adds the option to set $idType on models and override the default type (int) which is currently assumed to be true.

This shouldn't break anything for anyone else, it just adds this extra option which helps people with non-int ID types that are generated by the database rather than the application.

Let me know if you have any thoughts, questions, etc. I'd love to get this merged in (or at least start a discussion on how to best handle the issue). Thanks!

[GrahamCampbell]

We should make it clear that this property is ignored when incrementing is not enabled.

[GrahamCampbell]

Please start a new line here.

[GrahamCampbell]

Actually, just make the description this: The type used by the incrementing ID.. It's pretty clear then.

[stevenleeg]

Any idea why the build is failing here? It's throwing a bunch of these:

Error: Call to a member function connection() on null

but I'm not entirely sure how this patch would be affecting anything to do with connections...

[GrahamCampbell]

$this->$idType ;)

[GrahamCampbell]

Any idea why the build is failing here? It's throwing a bunch of these:

Yeh, I've left a comment inline.

[GrahamCampbell]

but I'm not entirely sure how this patch would be affecting anything to do with connections...

Yeh, the error is a bit cryptic due to our __call usage.

[stevenleeg]

doh! it's probably something to do with that bad getter call.

[GrahamCampbell]

doh! it's probably something to do with that bad getter call.

Yeh, that was exactly it. ;)

[GrahamCampbell]

Looks like it should work now. Could you add a couple of tests please, then it'll be a 👍 from me.

[stevenleeg]

I wasn't entirely sure how to unit test this at first but I think I figured something out. Let me know what you think of the two added in the latest commit (or just merge if they seem 👍 ).

[acasar]

@stevenleeg If your model doesn't have an auto-incrementing integer key you should set:

public $incrementing = false;

I don't see any good use case for $idType.

[stevenleeg]

@acasar setting $incrementing = false won't allow us to get the id generated by the database back (see here) therefore we need to set $incrementing = true.

Currently this also causes ids to be typecasted toint, hence this PR. $idType is used to specify this type in order to prevent a bad typecast when an id column doesn't use ints.

[GrahamCampbell]

If your model doesn't have an auto-incrementing integer key you should set:

It can be auto-incrementing, but not an integer.

[acasar]

Ah yes, you're right :)

[stevenleeg]

@acasar glad we could clear things up :)

@GrahamCampbell is there anything else I can do/clear up to get this merged? also mega thanks for the quick feedback!

[GrahamCampbell]

@stevenleeg It looks good to me. It's up to Taylor to merge this though, if he likes it. ;)

[GrahamCampbell]

@stevenleeg Thanks for the contribution. ❤️

[rohansahai]

I could be wrong... but I think only the tests for this fix got merged into 5.2

Merge Commit -81a8482

(The tests pass because of the stub...maybe worth adding failure case tests?)

[rohansahai]

oops nvm I'm wrong, I just noticed $idType was renamed to $keyType for anyone else who runs into the same issue