Page MenuHomekolab.org

StatusPropertyTrait, and some more code de-duplication
ClosedPublic

Authored by machniak on Fri, Jan 7, 2:57 PM.

Details

Reviewers
mollekopf
Group Reviewers
Restricted Project
Commits
rK3d7439962063: StatusPropertyTrait, and some more code de-duplication
Summary

Silence phpstan errors

Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

machniak requested review of this revision.Fri, Jan 7, 2:57 PM
machniak created this revision.

I'm wondering whether there is a cleaner solution to relying on constants being defined in the class from the trait and the objectState function, both seems a bit magic. But since the object states do not mean the same thing between different objects, I can't think of a better solution either.

Check if isDegraded/degrade/undegrade can be incorporated into the trait as well, otherwise it looks good to me.

src/app/Jobs/WalletCheck.php
206

I suppose the call won't hurt, but checking for the method doesn't check if the entitleable is suspendable due to the trait (the method exists even if the constant doesn't).

src/app/User.php
420

Couldn't this function be split into isDegraded (which could be included in the trait), and isAccountDegraded(), which can check isDegraded first?

mollekopf requested changes to this revision.Mon, Jan 10, 10:51 AM
This revision now requires changes to proceed.Mon, Jan 10, 10:51 AM

Since traits can't define constants yet, one possible improvement would be to define interfaces, and use them together with trait(s). And we could unify the constant values to be the same for all objects at some point too. Still, User has STATUS_DEGRADED, Domain has STATUS_CONFIRMED, etc. So, for example:

interface StatusSuspendedInterface {
    const STATUS_SUSPENDED = 4;
    function isSuspended();
    function suspend();
    function unsuspend();
}

For now I prefer to not define interfaces and use my simple trait with the common status functionality.

src/app/Jobs/WalletCheck.php
206

That's right, but the method does nothing for objects that do not support suspend, so we're good.. I find this approach more generic and simpler.

src/app/User.php
420

It probably could, but separately from this diff.

Since traits can't define constants yet, one possible improvement would be to define interfaces, and use them together with trait(s). And we could unify the constant values to be the same for all objects at some point too. Still, User has STATUS_DEGRADED, Domain has STATUS_CONFIRMED, etc. So, for example:

interface StatusSuspendedInterface {
    const STATUS_SUSPENDED = 4;
    function isSuspended();
    function suspend();
    function unsuspend();
}

For now I prefer to not define interfaces and use my simple trait with the common status functionality.

I agree that we shouldn't go down the interface route for this, and that the change is too invasive for now (so doesn't block this patch).

FWIW, I think statics could also be used to work around the trait limitation, but that too doesn't seem like quite what we want and would result in a slight syntax change.

Regarding the allowed state per object, we'd have to specify for each object somehow which states are supported (separate traits/an array of allowed states/...)

Anyways, that's for another patch.

mollekopf accepted this revision.Tue, Jan 11, 7:55 PM
This revision is now accepted and ready to land.Tue, Jan 11, 7:55 PM
This revision was automatically updated to reflect the committed changes.