Model Visibility Scoping Extender and Tests

[askvortsov1]

Fixes part of #1891

Changes proposed in this pull request:

• Add extender and integration tests for model visibility scoping

Reviewers should focus on:
Several areas of potential concern:

• A BC layer is provided for any model visibility scopers registered via policies, and for calling whereVisibleTo. However, dispatching a ScopeModelVisibility event will only apply visibility scopers registered via the old system (policies). I think this is reasonable though, since the only use cases of dispatching ScopeModelVisibility I've found occur in core and bundled extensions.
• I had to add the --process-isolation flag to composer.json, because otherwise the visibility scopers are added statically EVERY TIME a test runs, which makes things very slow. This isn't a problem for actual web requests in production, since each request is handled separately. That being said, we should move away from static stuff as much as possible after the extender API is finished
• I had to add unrelated code to UserTest, to ensure proper state there before the test runs. We really need to get Run Backend Tests in Transactions #2304 fixed up and merged so these side effects don't matter anymore.

Confirmed

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

[clarkwinkelmann]

since the only use cases of dispatching ScopeModelVisibility I've found occur in core and bundled extensions.

https://github.com/clarkwinkelmann/flarum-ext-see-past-first-post/blob/master/src/Access/PostPolicy.php#L34 👀

While my code in see-past-first-post is short, it's dark magic for me. I have no idea how that really works and whether this PR would break it or not.

---

There seems to be only 3 third-party extensions importing the class. kyrne/evergreen only listens to it, and the third has been deleted from GitHub.

https://query.flarum.dev/q/8296c9a1-fa6f-48ca-b73e-96447cab9315

---

Looking through the PR, I'm a bit lost, but having the event out of the picture does seem to make the code a bit more logical to read.

[askvortsov1]

While my code in see-past-first-post is short, it's dark magic for me. I have no idea how that really works and whether this PR would break it or not.

You should be able to replace that line with:

Discussion::query()->setQuery($query)->whereVisibleTo($actor, 'seePastFirstPost')

There seems to be only 3 third-party extensions importing the class. kyrne/evergreen only listens to it, and the third has been deleted from GitHub.

Considering how complex it would be to provide BC for this, I think we should be fine just breaking it then.

Looking through the PR, I'm a bit lost, but having the event out of the picture does seem to make the code a bit more logical to read.

I'll add a writeup explaining the key changes, sorry should have done that to begin with.

One other concern I thought of: in the change to Notification.php, we assume that any subject model will necessarily be visiblity-scopable, but this isn't necessarily the case. I suppose we could check that the class in question has the proper methods?

[askvortsov1]

@clarkwinkelmann To briefly summarize changes:

Under the current model visibility scoping system, a developer can dispatch a ScopeModelVisibility event, which takes an Eloquent query builder, the current user, and an ability string. Model visibility scopers, which listen to the event, mutate the query (generally restricting or expanding access in some way or another).

We replace the event with a more-or-less standard, callback-based solution:

• Model visibility scoping callbacks take a query and user. They are registered to a model and ability
• There's also "defaultScopers", which are registered to a model, and run on EVERY scope for that model, regardless of the ability. They take the ability as a third argument.

Code that triggers scoping by calling whereVisibleTo does not need to be changed. However, code that triggers scoping by dispatching a ScopeModelVisibility event needs to call whereVisibleTo on that query object instead (with the ability as an optional second argument). Big portions of this PR just move visibility scopers out from policies into their own classes; they are unchanged except for the aforementioned event dispatch => whereVisibleTo change.

And then, of course, there's the extender and tests.

[SychO9]

I'm glad this moves us away from the magic of view => find and findWithPermission, the code is more clear and explicit now, easier to read.

The logic looks good to me, I only have a few minor code style comments.

[clarkwinkelmann]

Looking good code-wise!

I'm just a bit confused by the default naming.

First the pseudo-constant. Reading the past comments it's seems it's a workaround for not being able to use a real constant? But the way it stands right now it seems like it's intended to be modifiable by classes that extend it.

Since it's used in only two places (?) couldn't we just hard-code * ? If we leave it like it is now, I think a phpdoc comment with a proper explanation and @internal is in order.

Second I just don't think the name fits the feature. Default makes it sound like it will run if no other scoper runs, instead it really is a catch-all/listen-all. What about scopeAll / scopeAny for the name of the extender method?