Page MenuHomekolab.org

Use a UuidTrait instead of an observer
ClosedPublic

Authored by mollekopf on Aug 20 2021, 1:27 PM.

Details

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

Diff Detail

Repository
rK kolab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 36376
Build 14392: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Aug 20 2021, 1:27 PM
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).

mollekopf updated this revision to Diff 8032.Aug 20 2021, 9:36 PM

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

mollekopf updated this revision to Diff 8038.Aug 24 2021, 9:06 AM

Now including the binary variant

mollekopf updated this revision to Diff 8047.Aug 24 2021, 9:37 AM

Whitespace

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

src/app/Traits/BinaryUuidKeyTrait.php
14

We didn't check that before.

mollekopf updated this revision to Diff 8101.Aug 26 2021, 1:53 PM

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

mollekopf added inline comments.Aug 26 2021, 1:58 PM
src/app/Traits/BinaryUuidKeyTrait.php
14

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

machniak added inline comments.Aug 26 2021, 2:16 PM
src/app/Traits/BinaryUuidKeyTrait.php
14

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

mollekopf updated this revision to Diff 8110.Aug 26 2021, 2:19 PM

Renamed traits

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.

machniak accepted this revision.Aug 27 2021, 8:59 AM
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.