Adding dynamic accessors and mutators from $uuidAttributes and customising uuid text attribute suffix #49

isocroft · 2018-08-06T14:50:00Z

This PR references this PR by @westphalen and also this issue #45 and #33 . It also makes the default uuid text attribute suffix customizable with support for composite keys without breaking original code. Details are on README.md file on this PR branch.

Thanks in anticipation.

brendt

Great, thanks for the PR! There are a few naming issues I'd like to be solved, and I also had some implementation questions. Overall, it seems like a good way forward!

One major remark though: you should also add tests for this new feature!

Can you confirm that all the old code will keep working the way it did? Otherwise we're looking at a major version bump. Nothing wrong with that, but if it's the case, I'm also going to add another breaking change, and than this PR should target the v2 branch.

brendt · 2018-08-07T08:15:14Z

src/HasBinaryUuid.php

+
+        $array = parent::toArray();
+
+        // if (! $this->exists) {


Let's clean this up :)

brendt · 2018-08-07T08:16:07Z

src/HasBinaryUuid.php

+        // return $array;
+        // }
+
+        if (is_array($uuidAttributes)) {


I'd prefer an early return here. if (! is_array()) { return ... }

yes, you're right and i will do this... very good refactor suggestion

brendt · 2018-08-07T08:17:11Z

src/HasBinaryUuid.php

+
+        if (is_array($uuidAttributes)) {
+            foreach ($uuidAttributes as $attributeKey) {
+                if (array_key_exists($attributeKey, $array)) {


Also here, an early return allows for better readability:

if (! array_key_exists()) { continue; }

brendt · 2018-08-07T08:18:07Z

src/HasBinaryUuid.php

+        return $array;
+    }
+
+    public function getRelatedBinaryKeyName($attrib)


Use the full name $attribute, no need to use shortened names, they make things more confusing for people reading the code.

Exactly... totally agree

brendt · 2018-08-07T08:19:18Z

src/HasBinaryUuid.php

+
+    public function getAttribute($key)
+    {
+        if (($uuidKey = $this->uuidTextAttribute($key)) && $this->{$uuidKey} !== null) {


To make this more readable, you should avoid assignment in an if statement:

$uuidKey = $this->uuidTextAttribute($key); if ($uuidKey && $this->{$uuidKey} !== null)

brendt · 2018-08-07T08:29:32Z

README.md

+     * 
+     * @var
+     */
+    protected $uuidTextAttribSuffix = '_str';


This can be a more clear name. Possibly uuidSuffix ? Also see further down for more feedback on this name.

uuidSuffix is not clear to me. uuidTextAttribSuffix sounds clumsy. I would suggest textUuidSuffix or textualUuidSuffix.

Additionally, would this belong to configuration or model? Recently in a PR for laravel-status, @freekmurze made me use a configuration option for attribute names. You may want to look at your other packages to get something consistent in the whole Spatie ecosystem :)

I was actually stuck between trying to make the property name meaningful while keeping it shortest. You're totally right. This is better

brendt · 2018-08-07T08:32:57Z

src/HasBinaryUuid.php

+
+        $key = $this->getKeyName(); // ! composite primary keys will return an array
+
+        if (is_string($key)) {


I think this check can be removed by simply casting $key to an array:

$key = (array) $this->getKeyName(); $uuidAttributes = array_merge($uuidAttributes, $key);

brendt · 2018-08-07T08:33:54Z

src/HasBinaryUuid.php

+        $uuidAttributes = [];
+
+        if (property_exists($this, 'uuidAttributes')) {
+            $uuidAttributes = $this->uuidAttributes === null ? [] : $this->uuidAttributes;


$uuidAttributes = $this->uuidAttributes ?? [];

should work

brendt · 2018-08-07T08:34:32Z

src/HasBinaryUuid.php

+            $uuidAttributes = $this->uuidAttributes === null ? [] : $this->uuidAttributes;
+        }
+
+        $key = $this->getKeyName(); // ! composite primary keys will return an array


What if the user also specified the primary key name in $uuidAttributes, shouldn't doubles be filtered out?

The array_unique(array_merge($uuidAttributes, $key)); call will be used to filter out the duplicate(s) going by the way array_unique works

brendt · 2018-08-07T08:36:30Z

src/HasBinaryUuid.php

+        return (property_exists($this, 'uuidTextAttribSuffix')) ? $this->uuidTextAttribSuffix : '_text';
+    }
+
+    protected function uuidTextAttribute($key)


Do I understand this code correctly that when calling $model->uuid_text, the actual property's name is calculated, so the return value would actually be the textual version value of the primary key? You're adding this as a shortcut, I suppose?

Not a shortcut per say. What i'm doing here is to avoid hard coding ->uuid_text to the property name for textual UUIDs and to also make it customizable to that developers can choose another property name suffix apart from '_text'

yeah, that's fine.

vpratfr · 2018-08-07T09:19:22Z

To take this closer to Eloquent way of naming stuff, instead of protected $uuidAttributes, I would simply have protected $uuids

Just like we already have : protected $casts or protected $dates

brendt · 2018-08-07T09:25:19Z

Good point, though the correct name would be binaryUuids, no?

vpratfr · 2018-08-07T09:26:34Z

It is unlikely that you store binary and string UUIDs at the same time in the model. So I would go with the shortest option for simplicity.

isocroft · 2018-08-07T12:16:24Z

@brendt The only previously existing method in the HasBinaryUuid trait that was modified by this PR is toArray() and the modification was done transparently. The main aim of this modification was to provide for 3 things without changing the way toArray() originally worked before.

Add support for other uuid columns in the table for any Model using HasBinaryUuid besides the primary key uuid to be decoded to their UUID textual representation.
Add support for composite primary keys on any Model which happen to be uuids.
Add support for custom suffix for textual UUID property name(s) or attributes.

All other code modifications are additions/insertions that do not interact with previously existing methods in a stateful way. In the event that a developer duplicates related or primary keys in the $uuids array or in a composite primary key, all duplicates are removed before decoding commences.

I have adapted my code to the reviews given by @brendt and have included tests for the all features i have added. Also, this PR does not introduce any breaking changes. Therefore, the changes can still be released under the 1.x.x versioning line.

vpratfr · 2018-08-07T13:21:41Z

Are there any tests to check that it still works as expected with custom suffix, toArray serialization of attributes, etc. ?

isocroft · 2018-08-07T14:45:15Z

Yes there are tests to check the custom suffix @vpratfr .

isocroft · 2018-08-08T08:40:18Z

Any merge for this PR coming soon ?

brendt · 2018-08-08T13:39:32Z

Yes.

brendt · 2018-08-08T13:45:03Z

Released as 1.2.0: https://github.com/spatie/laravel-binary-uuid/releases/tag/1.2.0

isocroft · 2018-08-08T15:43:29Z

Super!

isocroft added 9 commits August 6, 2018 13:46

Update HasBinaryUuid.php

a40f1c0

Update README.md

26e6529

Update README.md

4c30451

Update HasBinaryUuid.php

6ba7989

Update HasBinaryUuid.php

36398bf

Update README.md

f242593

Update HasBinaryUuid.php

91a24db

Update HasBinaryUuid.php

6d8ea4f

Update HasBinaryUuid.php

79ae094

brendt suggested changes Aug 7, 2018

View reviewed changes

isocroft added 3 commits August 7, 2018 14:06

Update HasBinaryUuid.php

aed11d4

Update README.md

0ab5099

Update HasBinaryUuid.php

df01737

isocroft added 9 commits August 7, 2018 14:24

Update HasBinaryUuid.php

960abef

Update HasBinaryUuid.php

f55f58f

Update README.md

07c6bd9

Update TestModel.php

b77cbb5

Update TestModel.php

c11c14e

Update HasBinaryUuidTest.php

ae2126d

Update HasBinaryUuidTest.php

2e85c27

Update TestModel.php

3cc7650

Update HasBinaryUuidTest.php

1b40960

Update HasBinaryUuid.php

772d9ac

brendt approved these changes Aug 8, 2018

View reviewed changes

brendt merged commit b4e6802 into spatie:master Aug 8, 2018

brendt mentioned this pull request Aug 9, 2018

Error in HasBinaryUuid trait...? #51

Closed

isocroft mentioned this pull request Aug 12, 2018

Making _text available to other secondary UUID columns #45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding dynamic accessors and mutators from $uuidAttributes and customising uuid text attribute suffix #49

Adding dynamic accessors and mutators from $uuidAttributes and customising uuid text attribute suffix #49

isocroft commented Aug 6, 2018

brendt left a comment

brendt Aug 7, 2018

isocroft Aug 7, 2018

brendt Aug 7, 2018

isocroft Aug 7, 2018

brendt Aug 7, 2018

brendt Aug 7, 2018

isocroft Aug 7, 2018

brendt Aug 7, 2018

isocroft Aug 7, 2018

brendt Aug 7, 2018

vpratfr Aug 7, 2018

isocroft Aug 7, 2018

brendt Aug 7, 2018

brendt Aug 7, 2018

isocroft Aug 7, 2018

brendt Aug 7, 2018

isocroft Aug 7, 2018 •

edited

Loading

brendt Aug 7, 2018

isocroft Aug 7, 2018

brendt Aug 7, 2018

vpratfr commented Aug 7, 2018

brendt commented Aug 7, 2018

vpratfr commented Aug 7, 2018

isocroft commented Aug 7, 2018 •

edited

Loading

vpratfr commented Aug 7, 2018

isocroft commented Aug 7, 2018 •

edited

Loading

isocroft commented Aug 8, 2018

brendt commented Aug 8, 2018

brendt commented Aug 8, 2018

isocroft commented Aug 8, 2018


		$key = $this->getKeyName(); // ! composite primary keys will return an array

		if (is_string($key)) {

Adding dynamic accessors and mutators from $uuidAttributes and customising uuid text attribute suffix #49

Adding dynamic accessors and mutators from $uuidAttributes and customising uuid text attribute suffix #49

Conversation

isocroft commented Aug 6, 2018

brendt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isocroft Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpratfr commented Aug 7, 2018

brendt commented Aug 7, 2018

vpratfr commented Aug 7, 2018

isocroft commented Aug 7, 2018 • edited Loading

vpratfr commented Aug 7, 2018

isocroft commented Aug 7, 2018 • edited Loading

isocroft commented Aug 8, 2018

brendt commented Aug 8, 2018

brendt commented Aug 8, 2018

isocroft commented Aug 8, 2018

isocroft Aug 7, 2018 •

edited

Loading

isocroft commented Aug 7, 2018 •

edited

Loading

isocroft commented Aug 7, 2018 •

edited

Loading