Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate removing is_xhr check from accepts helper / renderer method #1199

Closed
jberger opened this issue Feb 23, 2018 · 3 comments
Closed

Comments

@jberger
Copy link
Member

jberger commented Feb 23, 2018

Currently the accepts helper (and the accepts method in the renderer that it delegates to) predicate the allowance of multiple values in the Accept header based on the value of is_xhr. This was necessary since by default browsers often used very messy Accept header which might often prioritize content types that were not ones that most users would actually prefer, like xml or even jpg. Meanwhile since xhr requests were created manually, those were more believable and it could be assumed that if multiple types were acceptable, they had been explicitly chosen as such.

This does mean that for non browser clients, requests with multiple prioritized Accept-able content types were ignored unless you lied and said that it was XHR or unless you rolled your own version of the accepts method. If modern browsers were providing better Accept headers then perhaps this restriction could be lifted.

MDN provides a list of the Accept headers as sent by modern browsers. By the current values on that list, we still couldn't drop the XHR header check since Chrome still Accepts xml over html. That said, the page appears to be out of date. I checked my Chrome (via perl -Mojo -E 'a("/" => sub { $_->render(text => $_->req->headers->accept) })->start' daemon) and got text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8. Even so, if even fairly recent Chrome versions (those likely to still be in the wild) still have this value, we probably can't drop the restriction.

Additionally it would be interesting to check what other modern frameworks do. It is my understanding that Mojolicious' check is (or at least was) in keeping with the standard practice of other major frameworks.

@kraih
Copy link
Member

kraih commented Feb 23, 2018

Looks like WebKit changed the default in 2011. https://bugs.webkit.org/show_bug.cgi?id=27267

@kraih
Copy link
Member

kraih commented Feb 23, 2018

Anyway, i don't see any reason for why we can't lift the XHR requirement and would welcome a patch. 👍

@KES777
Copy link
Contributor

KES777 commented Feb 24, 2018

In this discussion ruby on rails developers decide to drop support for old browsers and implement correct MIME detection as of 2013.

I think the backwards compatibility argument is flimsy.

Here is the post How to create list of accept-headers from browsers. This list is five years old but, as we can see, most browsers has text/html, ... in theirs Accept header. Today's it should be more clean.

@mojolicious mojolicious deleted a comment from KES777 Feb 24, 2018
@kraih kraih closed this as completed in f5441db Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants