Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

HasUuidPrimaryKey trait is redundant #34

Closed
westphalen opened this issue Jan 18, 2018 · 3 comments
Closed

HasUuidPrimaryKey trait is redundant #34

westphalen opened this issue Jan 18, 2018 · 3 comments
Assignees

Comments

@westphalen
Copy link

The HasBinaryUuid trait already assumes that the primary key of the model should be a binary UUID:

protected static function bootHasBinaryUuid()
{
    static::creating(function (Model $model) {
        if ($model->{$model->getKeyName()}) {
            return;
        }

        $model->{$model->getKeyName()} = static::encodeUuid(Uuid::uuid1());
    });
}

and also more or less expects the primary key name to be uuid (rather than id or whatever).

public function getUuidTextAttribute(): string
public function setUuidTextAttribute(string $uuid)

So HasUuidPrimaryKey trait doesn't influence whether the primary key will be treated as UUID, which is kind of bad, if you just want to use HasBinaryUuid trait for other attributes.
But even if the intention of HasUuidPrimaryKey is just an overkill way to change the primary key name from id to uuid, it makes no sense tha HasUuidPrimaryKey implements

public function getIncrementing()
{
    return false;
}

while HasBinaryUuid doesn't.

The best behavior would be for HasBinaryUuid to leave the primary key alone, unless HasUuidPrimaryKey is set.

@brendt
Copy link
Contributor

brendt commented Jan 18, 2018

We're open for a PR to change this 😊

@brendt brendt self-assigned this Jan 18, 2018
@slashequip
Copy link
Contributor

@brendt I guess this can be closed now as HasUuidPrimaryKey is deprecated?

@brendt
Copy link
Contributor

brendt commented Feb 13, 2018

Indeed, as of 1.1.5, HasPrimaryUuid is deprecated and will be removed in 2.0.

More information about it in this PR: #36

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants