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

The `$request->ajax()` Method Reports Incorrectly for Vanilla Javascript #1431

Closed
luxlogica opened this issue Feb 2, 2019 · 4 comments

Comments

@luxlogica
Copy link

commented Feb 2, 2019

Describe the bug
The $request->ajax() method is no longer reliable in detecting whether a request was made via ajax, and should perhaps be deprecated.

It seems all the method does is check for the presence of a "X-Requested-With" request header. As explained in this StackOverflow post this will only produce reliable results when the programmer is using a javascript library - like jQuery - to handle the ajax. This is a non-standard header which is added mostly only by such libraries. If the programmer is using vanilla javascript - including the new fetch() api - then this non-standard header is not set, and the method will wrongly report that the request is "not ajax".

To Reproduce
Steps to reproduce the behavior:

  1. create a custom route
  2. in your custom route, use $request->ajax(), to test whether the request is an ajax request and provide different responses
  3. in your web page, make an ajax request using fetch()
  4. watch in horror as Kirby thinks this is NOT an ajax request...

Expected behavior
The method should correctly detect whether a request was done via ajax.

Kirby Version
Version 3.0.0 - initial release.

Additional context
The fetch() api is already supported by all current major browsers - and has been for a while - and is gaining wide adoption. Unfortunately, with the advances in javascript, there no longer seems to be a reliable way to detect ajax calls - not unless the programmer is using a library that will add these non-standard headers. As more and more developers try to use vanilla javascript for simple tasks, it may be better to remove the method altogether, rather than provide it with increasing caveats.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

That's not really a bug. The problem is that the fetch request does not send anything that could be detectable by the server. There's no chance to find out if the request was made with fetch or not. The only solution is to send the x-requested-with header in your fetch request or define your own custom header and then make your own check on the server.

@luxlogica

This comment has been minimized.

Copy link
Author

commented Feb 2, 2019

There are numerous ways to manually check whether the request came from the intended origin - adding a custom header is just one, and not usually the recommended way. Overall, it seems the ajax() method is just not very useful, and quite misleading, because it makes the developer believe there is a ‘standard’ or ‘built-in’ way in modern JavaScript to check whether a request is Ajax. In reality, there isn’t, and the method is making lots of assumptions - I.e. that the programmer is either using a JS library like jQuery, with functions that will automatically add the non-standard header, or that the programmer will add the unnecessary header themselves.

So, the option is to either put a lengthy explanation/warning in the docs about how the method will fail in typical situations using standard, vanilla JS, or to deprecate it. Honestly, it is unnecessary. A cookbook recipe showing how to process a form submission sent with fetch api and processed with a custom route would be much more useful.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

I agree with @luxlogica that we should deprecate this method.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

The method is now marked as deprecated in the source and will also be listed as deprecated in the changelog in 3.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.