Page MenuHomePhorge

Use a UuidTrait instead of an observer
ClosedPublic

Authored by mollekopf on Aug 20 2021, 1:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 8, 8:28 AM
Unknown Object (File)
Mon, May 6, 9:43 PM
Unknown Object (File)
Tue, Apr 30, 10:53 AM
Unknown Object (File)
Fri, Apr 26, 5:29 PM
Unknown Object (File)
Thu, Apr 25, 10:37 PM
Unknown Object (File)
Thu, Apr 25, 4:59 PM
Unknown Object (File)
Thu, Apr 25, 12:34 PM
Unknown Object (File)
Apr 20 2024, 3:59 AM
Subscribers

Details

Reviewers
machniak
Group Reviewers
Restricted Project
Commits
rKcdfdbedb8393: Use a UuidTrait instead of an observer

Diff Detail

Repository
rK kolab
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mollekopf created this revision.
mollekopf added a reviewer: Restricted Project.Aug 20 2021, 3:54 PM

If it works, I like it. We should apply it to other models too. I'm not sure about the name, maybe UuidKeyTrait would be better?

We also have three models that use uuidInt. So, another trait?

Yes, the idea would be to apply this across the board.

UuidKeyTrait is indeed a better name I think, and for the int variant I would create another trait BinaryUuidKeyTrait or IntUuidKeyTrait (I like the former better).

Just the string variant so far, not handling a withTrashed issue.

Now including the binary variant

I think I would prefer UuidIntKeyTrait and UuidStrKeyTrait. Other than that it looks good.

src/app/Traits/BinaryUuidKeyTrait.php
14 ↗(On Diff #8047)

We didn't check that before.

Use the trait boot method so we can have multiple traits with boot methods

src/app/Traits/BinaryUuidKeyTrait.php
14 ↗(On Diff #8047)

But we need to if we want to make the trait compatible with softdelete models, right?

src/app/Traits/BinaryUuidKeyTrait.php
14 ↗(On Diff #8047)

I was referring to if (empty($model->{$model->getKeyName()})) {. There was no such check before.

I found one problem with this approach. The handlers registered by these traits are executed before observer's event handlers. I.e. we generate uuid before we do 'creating' pre-checks. For example EntitlementObserver. I think it makes sense to keep pre-checks in the observer, and do "simple" stuff via traits, but the order should be reversed so observers run first. Well, at least for these cases at hand it would be a proper way.

This revision is now accepted and ready to land.Aug 27 2021, 8:59 AM
This revision was automatically updated to reflect the committed changes.