notification method is_available() (possible) bug

[Senky]

Hi everyone,
I want to discuss usage of is_available() in notification methods. So far there is no method that uses auth class. So everything works normally, but I have added a new method which now looks like this:

Code: Select all

...
public function is_available()
{
	return $this->auth->acl_get('...');
}
...

So I wondered why it didn't send the notification sometimes and it turned out that is_available() is used in \phpbb\notification\manager::get_default_methods(). This method is used in almost all notification types to decide what notification methods will be used to notify users.

Now the problem is, that get_default_methods() uses sender's auth to decide what methods should receiver get. Therefore if sender is not allowed to use the method, receiver won't get the notification.

Can anyone tell me if this is really desired state and I should not be using auth in is_available() or it's a bug but no one cared yet because no one used auth yet?

[kasimi]

This looks like by design to me. The auth instance you get from the container always belongs to the user of the current request. The notification methods simply weren't built with support for user-depending availability in mind.

[Senky]

Oh I see that. But the semantics of the code clearly shows that receiver is the subject here. So I created the PR that at least somehow deals with it (by exposing receiver user ID):
https://github.com/phpbb/phpbb/pull/5511