Improve detecting permissions to view referenced discussions

[rob006]

Replaces #30

This PR fixes detecting permissions for discussions visibility, as $actor->can('viewForum', $discussion) sytnax is probably incorrect and did not worked for me.

It also improves behavior in case of lack of permissions or errors caused by other extensions:

1. Fallbacks user to Guest in case if request or actor was not passed to formatter - it should fix all issues for public forums.
2. Instead displaying "unknown discussion" placeholder, it leaves URL intact, so it is still clickable.
3. logs errors only when debug mode is enabled.

[rob006]

I think that there are different expectations from this extension, so I share my POV.

I installed this extension for backlink feature - it is really convenient in a forum used for gathering feedback, where sooner or later you get a bunch of duplicated discussions, which you want to link to the canonical one. Converting original links, so they display a discussion title, is a nice bonus, but right now it has way too many drawbacks:

1. Performance. Even server-side parsing is sub-optimal, since it queries discussion one by one, so 100 links = 100 extra queries. But that is acceptable, it should not happen that often. The same problem on JS level is way worse, but I can live with that too. But the fact that there is a visible delay between loading the original link and replacing it with the discussion title is too much - it shifts post content and it looks really ugly. I really think that applying this feature to old posts should be done by re-parsing old posts content by some console command and then relying on standard server-side logic, instead of having a worse substitute on JS level.

2. Placeholder for unavailable discussions. Replacing the original link with an unclickable placeholder is no-go in my case. Even if we ignore the possibility, that detection may be incorrect, you're just hiding useful information, like ID or slug, that may explain the intention of this link. There is also compatibility with other extensions or tools - some of them may operate on different permissions than consumer of output. For example, an external tool that converts discussion to PDF may not have permission to see a particular discussion, but a user that will read this PDF may have such permission, and hiding these links from him is a regression. Or extension that handles redirections - AFAIK it is not implemented yet, but it makes perfect sense to handle redirection if 2 discussions were merged, so deleted one will redirect to a target discussion. Right now this extension hides links, that may actually work. IMO this feature should be optional, and I strongly advise disabling it by default.

3. Logging. The inability to completely disable these logs for production is IMO unacceptable. Even skipping stack trace is not a solution, because this may still result in thousands of entries in the error log. I think that you're overestimating both the importance and usefulness of these logs, and it is mostly a result of an uncompromising approach for handling placeholders for unavailable discussion - right now bad things may happen if we have incompatible extensions because links no longer work. But it does not have to be like that - if we leave links intact, then nothing is broken, and it is safe to ignore this issue as long as the output is acceptable. And it should be acceptable in most cases since this is default Flarum behavior. ;) If you want to make it easier to debug this on production environment, then having a separate switch in settings to disable these logs is the way to go. Multiple extensions have something like that.

So to summarize my proposal:

1. Logging "errors" should be switchable in extension settings and disabled by default.
2. Replacing links to potentially inaccessible discussions with placeholder should be switchable in extension settings and disabled by default. This applies only to links in post content - current behavior for DiscussionReferencedPost is correct.
3. Fixing old posts should be done by re-parsing content using console command.

[rob006]

I have no idea how to fix Static method Flarum\Discussion\Discussion::whereVisibleTo() does not exist on this mock object error. I could try adjust the rest of the tests if we agree on desired behavior, but mocking this Laravel magic (or configuring real tests with database involved) is beyond me.

[n-peugnet]

I have no idea how to fix Static method Flarum\Discussion\Discussion::whereVisibleTo() does not exist on this mock object error. I could try adjust the rest of the tests if we agree on desired behavior, but mocking this Laravel magic (or configuring real tests with database involved) is beyond me.

I will do it. This is indeed tricky and I might also have pushed the limits of unit testing too far :sweat_smile:

So to summarize my proposal:

1. Logging "errors" should be switchable in extension settings and disabled by default.

2. Replacing links to potentially inaccessible discussions with placeholder should be switchable in extension settings and disabled by default. This applies only to links in post content - current behavior for DiscussionReferencedPost is correct.

3. Fixing old posts should be done by re-parsing content using console command.

Thank you for taking the time to write these detailed explanations, I get your points.

1. OK for logging only in debug mode. I could add an FAQ about why the rendering is incorrect that points to enabling debug mode.
2. You are right, removing the link may not be as important in posts' content as in the DiscussionReferencedPost. Adding an option for this is fine for me.
3. This can be added later. If you have some time you can open an issue about this, or else I will do it myself.

I will extract the "Guest changes" and point 1. from your pull request and merge then right now (started here: https://github.com/club-1/flarum-ext-cross-references/pull/32). And I will see what I can do for 2. that would be acceptable for you.

About this point specifically:

Converting original links, so they display a discussion title, is a nice bonus, but right now it has way too many drawbacks:

1. Performance. Even server-side parsing is sub-optimal, since it queries discussion one by one, so 100 links = 100 extra queries. But that is acceptable, it should not happen that often.

You are right, fetching all discussions in one request while rendering the posts in the backend would be a nice performance enhancement, and not that difficult to do.

[n-peugnet]

@rob006 I did this part of the changes in #32:

I will extract the "Guest changes" and point 1. from your pull request and merge them right now

If this is good for you, I will merge it now. I will then work on your second point and come back to you later today or tomorrow.