Skip to content

[2.x] Implement content negotiation for forum error handling#4677

Open
dsevillamartin wants to merge 5 commits into
2.xfrom
ds/2.x-forum-error-negotiation
Open

[2.x] Implement content negotiation for forum error handling#4677
dsevillamartin wants to merge 5 commits into
2.xfrom
ds/2.x-forum-error-negotiation

Conversation

@dsevillamartin
Copy link
Copy Markdown
Member

@dsevillamartin dsevillamartin commented May 31, 2026

Fixes #3850

Changes proposed in this pull request:

  • Create a error handler for the forum that determines whether to forward to JSON or HTML view
    • Use JSON API Formatter for forum errors if the request did not explicitly state text/html in Accepts header
    • Fixes /login route not returning JSON from forum
    • Fixes HighMaintenanceModeHandler not returning JSON for requests initiated by forum (uses isApiRequest, and our requests do not set the header)

Reviewers should focus on:
I added the bitworking/mimeparse package (last updated end of 2025) to parse more complex Accepts headers (eg. with quality priority). It looks like browsers basically always (wasn't able to find concrete evidence of this) have text/html in their Accepts. While our XHR requests have Accepts: */*.

However, this did make isHtmlRequest a weaker check that could be true at the same time as isApiRequest (due to the quality priorities). I think properly parsing with this package makes sense, however, perhaps isHtmlRequest should just be the negation of the stronger isApiRequest check (made this change)? Additionally, isHtmlRequest is only used in LogInController for the maintenance login error validaiton.

I also thought that adding this check to the forum-wide error handler made more sense than just the login route alone.
Plus, I didn't think just slapping an Accepts header to our JS requests was the proper solution.

Screenshot
image
image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@dsevillamartin dsevillamartin requested a review from a team as a code owner May 31, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LogInController doesn't handle validation exceptions properly

2 participants