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

add getIdAttribute method #12

Closed
wants to merge 2 commits into from
Closed

add getIdAttribute method #12

wants to merge 2 commits into from

Conversation

faustbrian
Copy link
Contributor

@faustbrian faustbrian commented Nov 30, 2017

This allows to continue to use the id attribute as usually but will return the uuid_text attribute.

$user->id will be the same as $user->uuid_text

@freekmurze
Copy link
Member

freekmurze commented Nov 30, 2017

Seems nice, your thoughts @brendt ?

@faustbrian could you also update the readme, mentioning the new behaviour?

@faustbrian
Copy link
Contributor Author

Added it to the human-readable section.

@freekmurze
Copy link
Member

@faustbrian Thanks! Could you also add a test to prove that it works?

@brendt
Copy link
Contributor

brendt commented Nov 30, 2017

The workaround @faustbrian works but it hard-codes the id attribute. This works very well on an individual model, but putting it in the main trait can cause things to break if you're already using that id field in your model for other things.

We could add a separate trait which takes care of this, but I'd also like to suggest another solution, by adding the following to the HasBinaryUuid trait:

public function toArray()
{
    return array_merge(parent::toArray(), [$this->getKeyName() => $this->uuid_text]);
}

This solution doesn't require overriding multiple methods, and doesn't depend on a hard-coded name for the id attribute.

I've pushed a branch containing this solution here: https://github.com/spatie/laravel-binary-uuid/tree/json_encode-support

But I'd like to hear your thoughts first. @faustbrian am I correct that the above snippet also fixes your use case? What's your opinion on the two solutions?

@faustbrian
Copy link
Contributor Author

I purposely put it on the id attribute because it would allow to not change any code if you switch from the id column to uuid as the underlying methods would just give you the uuid in text form if you try to access the id field.

Looks good to me that solution, will just add my getIdAttribute via a trait myself like I do now.

@brendt
Copy link
Contributor

brendt commented Nov 30, 2017

I actually also see a use for the getIdAttribute for exactly the reason you give @faustbrian

@freekmurze Ok for you if we add both? I would put the getAttributeId in a separate trait though, but it's a good helper to have.

@freekmurze
Copy link
Member

freekmurze commented Nov 30, 2017

The toArray addition: 👍

I would put the getAttributeId in a separate trait though

Not a fan of having yet another trait. The trait wouldn't be correct when the key name isn't id.

@brendt
Copy link
Contributor

brendt commented Nov 30, 2017

Ok, see #18.

I'll be closing this PR then.

@brendt brendt closed this Nov 30, 2017
@faustbrian faustbrian deleted the patch-1 branch November 30, 2017 09:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants