Core: [Mentions] Formatter parsing + rendering requires Post instance

[datitisev]

Bug Report

Current Behavior
See https://github.com/FriendsOfFlarum/pages/issues/2#issuecomment-455890815
Mentions adds listeners to parse and render mentions, but assumes that the context passed to the Flarum Formatter is a Post.
Errors at https://github.com/flarum/mentions/blob/master/src/Listener/FormatUserMentions.php#L24

Expected Behavior
Mentions should not assume the context passed to the formatter (and Rendering/Parsing events) is a post.

Possible Solution
Add a check for $post being a Post or check if mentionsUsers is null.

[franzliedke]

Is the problem that $post is not really a Post (why?) or that mentionsUsers is null? What is the precondition leading to this case?

[datitisev]

The Flarum formatter context argument can be anything, it's not required to be a post. But when the events Parsing and Rendering are fired, mentions listens to those and acts as if the context is a post. The context seems to only be used in the event listeners, and it is supposed to be nullable, but the listeners don't check if null and tried to get properties of the context.

Parse/render https://github.com/flarum/core/blob/v0.1.0-beta.8/src/Formatter/Formatter.php#L58, only called "context".

[franzliedke]

Well, core (and bundled extensions, as far as I can tell) only ever send posts to that method. So the problem is that another extension (pages?) uses that method as well, with a different context?

[datitisev]

@franzliedke Yep. There's no check for the type of context, a simple check in those listeners for either instance of Post _or_ check if the property, mentionsUsers for example, exists before using it.

I'd prefer the second option because right now, flarum/mentions parses mentions in pages. See below (helloss is not a real user, test is)

[franzliedke]

I guess we can do a generic check for CommentPost instances in the extender that will replace this event.

[franzliedke]

(It might still use the event, but the extender will wrap it specifically for the use case of hooking into the rendering of posts.)

[datitisev]

@franzliedke So the rendering of mentions in other contexts, i.e. Pages, would no longer be a thing? I mean, this is easily bypassed by just providing new CommentPost, so I'd go for the bug prevention instead of the feature-preventing solution.

[franzliedke]

So the rendering of mentions in other contexts, i.e. Pages, would no longer be a thing?

Yes, I thought that was not intended?

[datitisev]

I don't see the trouble with it. It doesn't format it as a mention unless the user exists, and no notifications are created.
I personally see it as a feature, allowing mentioned users to be linked from just the text mention like in normal posts.

[franzliedke]

I'd rather have such a feature enabled explicitly if that's what an extension wants. Especially in these scenarios, where we have somewhat unexpected interactions between two extensions.

[tobyzerner]

Related: https://github.com/flarum/core/issues/1322