Starting PMs Broken with Next Flarum Release

[askvortsov1]

As of https://github.com/flarum/tags/pull/107#discussion_r551639080, the bug that allowed starting discussions without tags was patched, meaning that PMs started with Byobu will no longer work.

As a workaround, a global policy should be added that uses the permission introduced in https://github.com/flarum/tags/pull/111.

[tariqwalji]

Hey @askvortsov1 ,

I have a couple questions around how this policy is to work.

1) In the tags release, the code in question uses hasPermission: if ($actor->hasPermission($ability) && $actor->hasPermission('bypassTagCounts'))

Obviously hasPermission won't invoke any policy like 'can' does as it simply looks up the permission int he DB, so it cannot be overridden via a policy.

2) Is the intention to create a global or a model policy?

If it is a model policy, the issue here is that flarum/tags will make its changes before fof/byobu, which means is_private won't be set at the point the policy is executed.

[askvortsov1]

Hi!

For (1), the code you linked is from the default global policy for viewing and starting discussions without tags. This actually isn't used for determining whether discussions can be started, just as one heuristic for determining whether the "start discussions" button should be enabled.

The code that determines whether tag count validation should be bypassed is at https://github.com/flarum/tags/pull/111/files#diff-e4d68a6927eaf3204987a3d7f9a5894177aa455e49e6fc2bfa166fa452e8abc1R114-R117. This invokes $actor->can on a discussion object (which is the discussion being started) and the 'bypassTagCounts' ability. So, a DiscussionPolicy with a bypassTagCounts method should be used in the new Byobu policy.

(2) is a bit more challenging, although is_private isn't the issue here (other, non-byobu extensions such as approval use is_private). The way I envisioned the aforementioned policy working is by checking whether recipients are indicated, and if so, granting permission to bypass tag counts.

The issue is that recipients are set in the PersistRecipients listener, which will run after tag's SaveTagsToDatabase. Since request data is unavailable to the model policy, with the current setup there's no way of knowing whether the discussion being started is a byobu private discussion.

I've discussed this with the team, and since this feels like an edge case, we don't think this warrants reinventing an event listener priority system (Laravel's was removed since it typically leads to anti-patterns).

As such, there's a few potential workarounds:

• Replace the start discussion controller with an extended one, and determine whether a discussion is private there.
• Add (and use) a new controller for starting private discussions, put the logic for determining whether a discussion is private there.
• Use a middleware or read from the PHP global request info to determine whether the discussions is private.
• Add a service provider that:
  • Gets all listeners for Flarum\Discussion\Saving ($events->getListeners(\Flarum\Discussion\Saving::class))
  • Removes all listeners for this event ($events->forget(\Flarum\Discussion\Saving::class)
  • Adds the PersistRecipients listener (and maybe the DropTagsOnPrivateDiscussions listener too) first, then reattaches all previously attached listeners
  • In PersistRecipients, set some arbitrary attribute of the $discussion object to true if the discussion is a byobu private discussion. This is needed because right now, recipients will only be attached after saving.
  • Read from that attribute in the policy, use that to determine whether tag counts should be bypassed.

Personally, I prefer the last one.

[tariqwalji]

Thanks for your response!

I was originally leaning towards the first option as a solution.

However I find your last option a little bit more intriguing and would then allow for the model policy to function as expected.

Maybe the ability to re-order the event listeners could be an Extender feature in the future?

[askvortsov1]

Maybe the ability to re-order the event listeners could be an Extender feature in the future?

Possibly, I think it would depend on how frequently we see this issue: this is the first time I've bumped into it. It wouldn't be too challenging to implement (the extender API helps a lot with that, a priority could be an optional 3rd argument), but I also feel like it might encourage some anti-patterns where an alternative solution would be cleaner and safer.