Deprecate GetModelIsPrivate and its usages

[askvortsov1]

Fixes part of #1891
Fixes #2420

Changes proposed in this pull request:
This PR deprecates the GetModelIsPrivate event; instead, extensions that need to set this should listen to the discussion Saving event, and set is_private there. I see no reason to treat is_private differently than other attributes of models. It's also only present on 2 models (without plans to be included on others, as far as I can tell). A full extender doesn't really make sense.

This will affect the downstream approval and byobu extensions, possibly others (I think php import query on https://query.flarum.dev might be broken).

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

Required changes:

• Related documentation PR: (Remove if irrelevant)
• Related core extension PRs: (Remove if irrelevant)

[SychO9]

For reference, the reason why this event was added: #1153 (comment)

With the event, if you have multiple extensions changing the value of is_private, the attribute takes the value of the first non-null response, regardless of what the other extensions set it to.
https://laravel.com/api/6.x/Illuminate/Events/Dispatcher.html#method_until

..and then both extensions would listen and if either of them return true then the discussion is considered private.

So while this seems to be the intended behavior, I don't think it's working like that, right now it's more of a first come first serve basis. (well.. first non-null)

While using the Saving event would work differently, I don't think it would impact approval and byobu (being the only two extensions that use this event).

[askvortsov1]

Thanks for finding that! I remember something like that being discussed a while ago in an internal meeting but didn't know there was a physical issue to go along with it :D

At the end of the day, it's been a few years and we only have 2 extensions using is_private (one of them bundled). And right now the only thing this is doing is switching the order of evaluation so chosen value of is_private is that returned by the first extension instead of the last extension as would happen with Saving.

I'm still skeptical that the edge case of multiple extensions changing is_private at the same time is applicable and needs its own extender, especially since other extensions that use private messaging (Kyrne's Shout, Xelson's chat, etc), have opted for dedicated models for messages.

@luceos I know you were involved in setting this system up. What are your thoughts on this change?

[luceos]

Approval doesn't use is_private. But back then it was very much intentional for us to add that flag to core, because you need a central place that defines whether a discussion/post is public.

I think we need to keep the event and create an extender around that. As correctly pointed out by @SychO9 we cannot rely solely on the Saving (or even serialization event) because we need a continued check whether something is private or not. We decided on this method to be absolutely sure something private will never show up publicly even when an extension making something private (like byobu) is disabled/uninstalled.

[SychO9]

Approval doesn't use is_private.

https://github.com/flarum/approval/blob/28960560bd28b8a4dc5f33e6b8b651720cc32b74/src/Listener/UnapproveNewContent.php#L67-L79

If the intended behavior is as toby says in the above quote in my previous comment. Then what about having just a method to do this instead of a whole extender ?

Using a method, either the setIsPrivateAttribute(bool) magic method, or a new setIsPrivate(bool) that extensions would have to use (I would prefer a normal non-magic method), it would look something like this:

public function setIsPrivate(bool $value)
{
    static $hasBeenSetTrue;

    if ($value) {
        $hasBeenSetTrue = true;
    }

    $this->is_private = isset($hasBeenSetTrue) || $value;
}

We would have no need for an extender, extensions would just use the Saving Event and either directly change the is_private attribute (if we used a magic method) or use the method (if not a magic one).

$discussion->setIsPrivate(true)

[clarkwinkelmann]

I don't know much about that method, having never used it myself.

Regarding the until issue, I don't think it's a real issue. But if we keep it like it's now, we need to update the docblock to say extensions should probably never return false.

I do agree that since the GetModelIsPrivate event is always triggered at the same time as a Saving event, it doesn't really need to be an event.

I quite like @SychO9 's idea about a method on the model that extensions could call during Saving.

But there's the important difference of the Eloquent saving event and the Flarum model saving event. The saving event might not be called by extensions doing direct model manipulation, while the Eloquent "event" (and as such GetModelIsPrivate) would be.

I think coming up with the extender first would make more sense, before starting to deprecate or remove code behind the scenes.

A great use case of events is the ability for extensions to trigger the events themselves. An extension could trigger the event to get knowledge about other extensions without actually saving the model. Switching to the Saving event or a callback-based extender would remove that ability. But I suppose that's what's happening with most of the events we've been removing lately...

[askvortsov1]

I think coming up with the extender first would make more sense, before starting to deprecate or remove code behind the scenes.

This is one of the things I was hoping to remove without an extender as replacement. I feel like it's way to specialized for a dedicated extender. But the Eloquent saving event concern is a good point... I suppose we could introduce this as separate methods on the Post and Discussion extender, or move that code out into a trait, and use a ModelPrivate extender?

[SychO9]

What's left to decide here ?

...or move that code out into a trait, and use a ModelPrivate extender?

Seems like we have no choice but to create an extender so, I would be in favor of this, could also just add a method in the Model extender.

[askvortsov1]

Yeah at this point i think it's a matter of implementation. I wouldn't want to add it to Model since not all models qualify for it; a trait similar to what's done with visibility scoping would probably be the way to go.

I'll update the PR sometime this week.

[askvortsov1]

Superceded by #2587