Reduce Interface Param Types

[luke-]

We recently introduced new interfaces "Viewable", "Readable" and more with the v1.16. https://github.com/humhub/humhub/blob/develop/protected/humhub/interface

For methods which requires an user, many possible parameter types (e.g. string, int, User, null) are allowed. e.g. https://github.com/humhub/humhub/blob/develop/protected/humhub/interfaces/ViewableInterface.php#L22

I suggest to remove this, and reduce to ?User . Even when this requires some refactoring.

@marc-farre @martin-rueegg @yurabakhtin What do you think about this?

[yurabakhtin]

@luke- Yes, I think we can reduce the params to ?User because in most cases we user $user === null, so we will need to update several cases where user ID is passed.

Also I think we could remove the interface Readable completely as I have written here.

[martin-rueegg]

@luke-, @yurabakhtin

I agree that we could reduce the code to ?User and also support the change to merge Readable and Viewable. I guess I've created them. And I did not have the big picture on whether or not they could be merged. But they imply the same thing. And keeping both would require documentation as to which to use when.

Also, as @luke- and I have discussed, the idea was to enhance the User|Group::find() under the hood, so they would allow to translate all those id|string|User and what have you into ?User. So the data would me converted into an object at the stage where it comes into appearance (from web client or database or cache or wherever) and then it's simply passed on as user object.

Like so, it is also covered by runtime cache, as discussed in the in the enhanced find() version.

I hope I will have some time next week to update the PR for the find methods:

• See #6503 series

[luke-]

Thank you for your quick feedback. Then lets reduce the parameter types for the interfaces.

@martin-rueegg Yes, the efforts with the flexible "find", which supports different parameter types (self, id, guid, null) is nice. But of course it is always better to work with clearly defined types. Otherwise we would have to use the User::find() wrapper in too often...

• What about the naming of the interfaces? For me, for example, Viewable is very generic. Here I would expect something visualizable/displayable/ with the first intention. I don't like the naming of CanViewInterface and CanEditInterface either. Do you have any ideas?

• Maybe we should move some interfaces e.g. `Archiveable' to this namespace? https://github.com/humhub/humhub/tree/develop/protected/humhub/modules/content/interfaces

[martin-rueegg]

@luke- Thank you for giving the opportunity to provide any feedback. :-)

Yes, the efforts with the flexible "find", which supports different parameter types (self, id, guid, null) is nice. But of course it is always better to work with clearly defined types. Otherwise we would have to use the User::find() wrapper in too often...

I might have expressed myself poorly. My intention was not to suggest to use User::find() all over the place. But rather in places where you would expect e.g. an id or guid to retrieve a User object which can then be passed on. That would be typically in model when validating form data. Then, in all other places, using ?User would be the "standard" to pass the user between methods and objects, rather than having guids and id's everywhere. As such, the usage of User::find() would actually be massively reduced, as the receiving method already gets whatever information it may require (i.e. id, guid, username, etc.) without having to do another find() call.

What about the naming of the interfaces?

Honestly, I would keep the names as they are. "read" is a technical term in IT that would maybe be more generic, as in read/write. But given that we also distinguish between write and delete (which e.g. in unix file systems is not distinguished in the same way) the triple view/edit/ delete seems a good fit to me. And "Viewable" says exactly what it means. Otherwise, we could use the term "AccessInterface", e.g. ViewAccessInterface and EditAccessInterface, etc.. However, since there are also additional methods, e.g., DeletableInterface::delete() or ArchiveableInterface::archive()/::unarchive(), the "Access" part seems too limiting.

Maybe we should move some interfaces e.g. `Archiveable' to this namespace? humhub/modules/content/interfaces

Actually, I would not do that. There might be other objects, not only content, that is "archivable". And you don't want to have them dispersed everywhere. But if you'd want, maybe you could group them in \humhub\interfaces\access or something along those lines. But then the same issue as discussed above comes into play.

I'd leave them, how and where they are. That's why I created them there in the first place. So my opinion is biased.... :-D

[luke-]

@luke- Thank you for giving the opportunity to provide any feedback. :-)

Yes, the efforts with the flexible "find", which supports different parameter types (self, id, guid, null) is nice. But of course it is always better to work with clearly defined types. Otherwise we would have to use the User::find() wrapper in too often...

I might have expressed myself poorly. My intention was not to suggest to use User::find() all over the place. But rather in places where you would expect e.g. an id or guid to retrieve a User object which can then be passed on. That would be typically in model when validating form data. Then, in all other places, using ?User would be the "standard" to pass the user between methods and objects, rather than having guids and id's everywhere. As such, the usage of User::find() would actually be massively reduced, as the receiving method already gets whatever information it may require (i.e. id, guid, username, etc.) without having to do another find() call.

Fully agree.

What about the naming of the interfaces?

Honestly, I would keep the names as they are. "read" is a technical term in IT that would maybe be more generic, as in read/write. But given that we also distinguish between write and delete (which e.g. in unix file systems is not distinguished in the same way) the triple view/edit/ delete seems a good fit to me. And "Viewable" says exactly what it means. Otherwise, we could use the term "AccessInterface", e.g. ViewAccessInterface and EditAccessInterface, etc.. However, since there are also additional methods, e.g., DeletableInterface::delete() or ArchiveableInterface::archive()/::unarchive(), the "Access" part seems too limiting.

If I understand correctly, we want to delete Readable and just use Viewable since canView() is mainly used. For an application wide Viewable interface I would expect something like View rendering or similar. Not just a single canView() method.

I am fine with the naming for interfaces like Archivable, Deleteable which are implementing multiple methods and true deletion or archive logic like archive().

I am unsure about Readable and Editable as they only contain the Access logic. So maybe we really should use something like your suggested naming like ViewAccessInterface and EditAccessInterface.

Maybe we should move some interfaces e.g. `Archiveable' to this namespace? humhub/modules/content/interfaces

Actually, I would not do that. There might be other objects, not only content, that is "archivable". And you don't want to have them dispersed everywhere. But if you'd want, maybe you could group them in \humhub\interfaces\access or something along those lines. But then the same issue as discussed above comes into play.

I'd leave them, how and where they are. That's why I created them there in the first place. So my opinion is biased.... :-)

Ok, since the interfaces don't use content specific classes, I would be fine with that. And indeed, Spaces can also be archived, edited, ... for example. (Even if these are ContentContainers :-))

[martin-rueegg]

Just some brainstorming:

We could create an AccessInterface with the following base methods:

• canAction(?User, 'action') is a generic access checker. List of actions could be set as constants in the interface.
• canWhat() would return a list of actions that can be queried by canAction()

And maybe:

• canView(?User)
• canEdit(?User)
• canDelete(?User) as well?

Then, DeletableInterface and ArchivableInterface inherit from this. (And also the current other interfaces, that will be marked deprecated.)

The AccessTrait would provide a default implementation of the required methods, always returning false/empty list. Some traits already exist (IIRC for archiving and deleting).

Maybe the ViewableInterface will have viewHtml() and viewArray() at some point. Or the EditableInterface will have an edit(array) method, for simple key=>value editing. Then it can be un-deprecated again.

[marc-farre]

@martin-rueegg thanks very much for your work!

Yes, I totally agree, ?User is better. I'll check all my modules.

We should also use the $user param here (we are checking only against Yii::$app->user->id): https://github.com/humhub/humhub/pull/6451/files#diff-8f222297fc38a2b61e2b6757afe50189314b7cd78d730657ae20590b8ceee73dR144

When possible, we should compare with === instead of ==.

Maybe could take this opportunity to also standardize https://github.com/humhub/humhub/blob/e5e46018cca67fcb1d4bd4e5dd63e58f9839c907/protected/humhub/modules/user/models/Group.php#L326-L326 and https://github.com/humhub/humhub/blob/e5e46018cca67fcb1d4bd4e5dd63e58f9839c907/protected/humhub/modules/space/behaviors/SpaceModelMembership.php#L49-L49

I suggest to replace SpaceModelMembership::isMember($userId = '') with SpaceModelMembership::isMember(?User $user) and add the ?User to 'Group::isMember($user)`.

But this should be of course another issue.

[martin-rueegg]

Yes, isMember is a good candidate, too. And there are of course many other places, but most certainly al can* methods should be revised at once to avoid incongruencies or having devs expecting a different method signature. E.g. there are also

• Space::canJoin($userId = '') https://github.com/humhub/humhub/blob/ae8f2fa34255a33aff639fb5a5cfde65d39a33c9/protected/humhub/modules/space/models/Space.php#L348
• Space::canJoinFree($userId = '') https://github.com/humhub/humhub/blob/ae8f2fa34255a33aff639fb5a5cfde65d39a33c9/protected/humhub/modules/space/models/Space.php#L348

And there are a few occasions of method(User $user = null) that should be rewritten to either method(?User $user = null) or method(User $user). E.g.

• ContentContainerActiveRecord::getUserGroup(User $user = null) https://github.com/humhub/humhub/blob/ae8f2fa34255a33aff639fb5a5cfde65d39a33c9/protected/humhub/modules/content/components/ContentContainerActiveRecord.php#L350

In a sense, it is also inconsistent that the can*() methods expect/require a User object, while the method can() itself does not: https://github.com/humhub/humhub/blob/ae8f2fa34255a33aff639fb5a5cfde65d39a33c9/protected/humhub/modules/content/components/ContentContainerActiveRecord.php#L297

[luke-]

@martin-rueegg Thanks for the ideas. The idea with canAction(?User, 'action') is nice and generic, but I would like to keep this simple as possible.

@marc-farre @martin-rueegg @yurabakhtin Are you okay with following plan? Do you prefer EditAccessInterface or CanEditInterface or any other idea?

• ArchiveableInterface
  • canArchive(?User): bool
  • archive(): bool
  • unarchive(): bool
• DeletableInterface
  • canDelete(?User): bool
  • delete(): bool
• EditAccessInterface | CanEditInterface
  • canEdit(?User): bool
• ViewAccessInterface | CanViewInterface
  • canEdit(?User): bool
• ~~ReadableInterface~~
• Other CanSomething Methods refactor on long run to CanSomething(?User): bool Method
  • canJoin(), isMember(), it's planed to create a new Space Membership Service which refactors these old methods
• ContentContainer::can(Permission, Params, allowCaching) - is a special case, need to think about it

[martin-rueegg]

Are you okay with following plan? Do you prefer EditAccessInterface or CanEditInterface or any other idea?

I generally think that's a good approach!

I don't like none of the EditAccessInterface or CanEditInterface names particularly. :-)

Is there any object that can be viewed (or not), but not edited? Or vice versa?

If they always go along together, I would still use one single interface (AccessInterface or BaseAccessInterface) with both canView(?User) and canEdit(?User) methods. And derive the other two ( ArchiveableInterface and DeletableInterface) from this, which basically indicates both: a) an AccessInterface and b) the possibility of the specific functionality (delete/archive).

[luke-]

Are you okay with following plan? Do you prefer EditAccessInterface or CanEditInterface or any other idea?

I generally think that's a good approach!

I don't like none of the EditAccessInterface or CanEditInterface names particularly. :-)

Is there any object that can be viewed (or not), but not edited? Or vice versa?

Yes, we have some situations:

• Comments doesn't implement a own canView() handling. This is related to a Content. If you can see the Content, you can also view all comments.
• Contents shouldn't implement a canView() handling, because this is always done via ContentContainer and Public/Private flag.

But it should not be a big deal to always implement canView() and canEdit(). So I'm also happy with your suggestion.

Hmm, I'm not sure we should derive ArchivableInterface from BaseAccessInterface.

So lets go with:

• ArchiveableInterface > BaseAccessInterface
  • canArchive(?User): bool
  • archive(): bool
  • unarchive(): bool
• DeletableInterface > BaseAccessInterface
  • canDelete(?User): bool
  • delete(): bool
• BaseAccessInterface
  • canEdit(?User): bool
  • canView(?User): bool
• ~~EditAccessInterface | CanEditInterface~~
  • ~~canEdit(?User): bool~~
• ~~ViewAccessInterface | CanViewInterface~~
  • ~~canView(?User): bool~~
• ~~ReadableInterface~~
• Other CanSomething Methods refactor on long run to CanSomething(?User): bool Method
  • canJoin(), isMember(), it's planed to create a new Space Membership Service which refactors these old methods
• ContentContainer::can(Permission, Params, allowCaching) - is a special case, need to think about it

[martin-rueegg]

I just noticed, that somewhere along the way, the following CHANGELOG entry got lost:

- Enh #6451: Introduce Archiveable, Deletable, Editable, Readable, & Viewable Interfaces

Is that by intention, or shall I create a PR to re-add it?

Btw and a bit off-topic: is the following correct in terms of branches and versions?

• v1.15 master
• v1.16 develop
• v1.17 next If so, we could update the version tag in config/common.php and CHANGELOG-DEV.md in next accordingly?

[luke-]

@yurabakhtin Can you please take a look into my refactor summary 2 comments above? Then we should also add the lost CHANGELOG entry.

@martin-rueegg Yes, you're correct with the branches. Currently next is not really used. This will become relevant when the v1.16 release comes closer :-)

[yurabakhtin]

@luke-

Can you please take a look into my refactor summary 2 comments above?

Ok, I agree.

Then we should also add the lost CHANGELOG entry.

Should I add the following line into CHANGELOG of the develop branch?

- Enh #6451: Introduce Archiveable, Deletable, Editable, Readable, & Viewable Interfaces

[martin-rueegg]

Should I add the following line into CHANGELOG of the develop branch?

Yes, that would be good as it is already merged into develop.