Handle post rendering errors to avoid bricking

[askvortsov1]

Fixes Part of flarum/issue-archive#85

Changes proposed in this pull request:
Whether it's due to corrupted content, missing tags, caching issues, or other assorted reasons, post content can't be rendered. Currently, this results in an exception that crashes the entire forum and is hard to debug. Instead, we should log the error and show an indicator message that rendering has failed.

Reviewers should focus on:
Is the Formatter class level the best place to do this? I also considered handling it directly under the relevant CommentPost method. The main benefit of handling per-call-site is that we could include some metadata (like the post ID) in the error message. However, we'd need to handle every possible use case, which is expensive and isn't worth it IMO.

Also, any suggestions on better error text?

Screenshots

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]

Yes! more safe erroring!

I wonder if we could have information on the frontend about the post failing to render so that we can have minimal visuals indicating a formatting failure rather than it looking like an ordinary post. Maybe if we were to add a $renderingFailed property to the comment post model and add that to the serializer ?

[askvortsov1]

Yes! more safe erroring!

I wonder if we could have information on the frontend about the post failing to render so that we can have minimal visuals indicating a formatting failure rather than it looking like an ordinary post. Maybe if we were to add a $renderingFailed property to the comment post model and add that to the serializer ?

We could, but that would make the formatter aware of what it's formatting, or require use-case-specific handling. That being said, supporting this is another good reason for moving this handling out of the general formatter and into the serializer.

[SychO9]

Ah I got overexcited and missed the PR description 🙈, I see it now.

Is the Formatter class level the best place to do this? I also considered handling it directly under the relevant CommentPost method. The main benefit of handling per-call-site is that we could include some metadata (like the post ID) in the error message. However, we'd need to handle every possible use case, which is expensive and isn't worth it IMO.

Like I mentioned above, I don't think metadata is needed as much as just a simple boolean value to indicate a rendering failure to the frontend. A simple visual change would be clean and a more efficient indicator of failure alongside the text. So moving this to either the model method or the serializer makes sense.

[askvortsov1]

Updated, here's how it looks like now: