Add 'type' to user blocking

[TimWolla]

Still a draft, because the type needs to be integrated into the consumers.
For now only the blocking UI has been adjusted.

---

Resolves #3981

[dtdesign]

LGTM

[TimWolla]

I believe I made all the changes required for WoltLab/WCF, so this one is ready to review.

[dtdesign]

I do not like the use of the class parameter constant for the "type" of blocking, it clearly shows its ugly face in templates. Instead, I would like to see this getting split up into two methods:

1. isIgnoredByUser($userID) - Behaves as before, full block (equals TYPE_HIDE_MESSAGES).
2. isBlockedByUser($userID) - Equals TYPE_BLOCK_DIRECT_CONTACT.

The naming ain't perfect, but it is close enough to both preserve backwards compatibility and enables cleaner code by avoding magic numbers.

[TimWolla]

The naming ain't perfect, but it is close enough to both preserve backwards compatibility and enables cleaner code by avoding magic numbers.

Should I preserve the $type parameter and just make isBlockedByUser a small wrapper around isIgnoredByUser, passing an appropriate $type for use in templates?

[dtdesign]

Should I preserve the $type parameter and just make isBlockedByUser a small wrapper around isIgnoredByUser, passing an appropriate $type for use in templates?

Can't we just drop the $type parameter? It does create some redundancy, but I think that is acceptable.

[TimWolla]

Can't we just drop the $type parameter? It does create some redundancy, but I think that is acceptable.

For backwards compatibility the existing methods must not perform any filtering, because in some places they are used to check whether the other user is blocked without caring exactly about what type of block. e.g.:

WCF/com.woltlab.wcf/templates/user.tpl

Lines 52 to 54 in 9817fda

	{if !$user->isIgnoredUser($__wcf->user->userID)}
	new UiUserProfileMenuItemFollow({@$user->userID}, {if $__wcf->getUserProfileHandler()->isFollowing($user->userID)}true{else}false{/if});
	{/if}

, or

WCF/com.woltlab.wcf/templates/user.tpl

Line 49 in 9817fda

new UiUserProfileMenuItemIgnore({@$user->userID}, {if $__wcf->getUserProfileHandler()->isIgnoredUser($user->userID)}true{else}false{/if});

Even if TYPE_HIDE_MESSAGES matches all types of blocks as of right now, this might hold true after a simple refactoring, thus allowing for accidentally breaking the behavior in the future.

IMO an explicit $type parameter would be the simplest safe solution here.

[dtdesign]

Okay, then I would like to suggest to keep the method isIgnoredUser() as it is, returning true if the user is blocked, regardless of what setting is enforced. Do we even need to distinguish between these types for the reverse lookup? isBlockedByUser(int $userID): bool should be sufficient to prevent any notifications from being generated and things like a direct contact / follow. The content blocking is not relevant for a reverse lookup.

[TimWolla]

isBlockedByUser(int $userID): bool should be sufficient to prevent any notifications from being generated and things like a direct contact / follow. The content blocking is not relevant for a reverse lookup.

Indeed. This might even leak some data that is considered private. I'll fix this and will think about the rest.

[TimWolla]

Okay, I looked into this again. I don't think we are able to easily get rid of the magic number in templates:

• getIgnoredUsers() / isIgnoredUser() cannot be modified for compatibility reasons and thus must return all blocked / ignored users regardless of type.
• Most conditions in the template want to know whether messages should be hidden, thus the users with type = direct contact need to be excluded.

Currently this exclusion is performed by explicitly passing the 2 magic number as the filter parameter. A helper method could theoretically be added, but naming is hard and the obvious name (“isIgnoredUser”) is taken. A “isBlockedUser” method would be useless, because this is rarely, if ever needed in templates – we want to get the users for whom messages should be hidden.

Thus the choice is between:

• Magic '2' in templates, and
• Awkwardly named helper method (suggestions?).

[ghost]

@TimWolla You can add return types to the new methods in the action class.

[TimWolla]

@TimWolla You can add return types to the new methods in the action class.

I feel that array is pretty much useless there, especially since the methods are never called directly. I'd rather wait for more useful type support.

Obsolete

[ghost]

I think that instead of the delete icon on the page listing the currently blocked users, the dialog should be shown for also being able to change the setting.

[TimWolla]

I think that instead of the delete icon on the page listing the currently blocked users, the dialog should be shown for also being able to change the setting.

I'll create a follow up for this: #4140