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

Bug: IsAJAX() relies on inconsistent headers #2454

Closed
MGatner opened this issue Dec 16, 2019 · 23 comments
Closed

Bug: IsAJAX() relies on inconsistent headers #2454

MGatner opened this issue Dec 16, 2019 · 23 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Milestone

Comments

@MGatner
Copy link
Member

MGatner commented Dec 16, 2019

Newer JavaScript implementations (i.e. fetch) don't set the HTTP_X_REQUESTED_WITH header, making the current implementation of IncomingRequest::isAJAX() unreliable.

So far I have not found a reliable way to distinguish fetch requests, and if this is ultimately the case then isAJAX() should be removed or reworked so it is clear that it is not definitive.

Ref. https://stackoverflow.com/questions/44202593/detect-a-fetch-request-in-php

@MGatner MGatner added the bug Verified issues on the current code behavior or pull requests that will fix them label Dec 16, 2019
@MGatner MGatner changed the title Bug: IsAJAX() relies on legacy headers Bug: IsAJAX() relies on inconsistent headers Dec 16, 2019
@MGatner
Copy link
Member Author

MGatner commented Dec 16, 2019

A temporary workaround is to add the header onto the fetch call, if you're writing it yourself:

fetch(url, {
    method: "get",
    headers: {
      "Content-Type": "application/json",
      "X-Requested-With": "XMLHttpRequest"
    }

@lonnieezell
Copy link
Member

Oh, well that's a giant PITA.

@MGatner
Copy link
Member Author

MGatner commented Dec 16, 2019

Right? With so many things moving to "pure" JavaScript fetch is now becoming a standard. I pretty much gave up, looks like there is no way to differentiate fetch calls unless the developer specifies it. I'd be curious to see how other frameworks are dealing with this.

@lonnieezell
Copy link
Member

Did a quick search and didn't find any other ways. So I guess the right thing to do here is to update the user guide to tell them it is not reliable, and provide an example for how to send the header along with their requests.

I'm tempted to rip it out altogether, but if they send the header it still works, and still works with "legacy" javascript solutions, like jQuery, etc. so probably not worth destroying yet. And I know people are already using the framework...

@MGatner
Copy link
Member Author

MGatner commented Dec 16, 2019

Is it too gross to change it to isXMLHttpRequest()? That makes it clear that it doesn't cover all AJAX requests but it will catch instances with the header set.

@lonnieezell
Copy link
Member

I think that's gross yes. :) But I also don't get too caught up on the name, because much of what we do is not "Asynchronous JavaScript and XML" anymore anyway. AJAX has become the umbrella term for that so I think it is still relevant and the most common name developers use.

Oh - and I don't hardly ever use it as an "XMLHttpRequest" either. More a "JSONHttpRequest" :P

@MGatner
Copy link
Member Author

MGatner commented Dec 16, 2019

Oh definitely, but I was referring to the header setting. Right now isAJAX() is pretty much if ($headers['X-Requested-With'] == 'XMLHttpRequest') so naming it after what the check actually is would make it clear what it does and doesn't do.

@lonnieezell
Copy link
Member

Yeah, I get that. I don't think it's necessary as long as there's a big section in the docs that addresses this.

I think it definitely warrants looking at how others are handling it (if at all), and a little more research before final release.

@lonnieezell lonnieezell added this to the 4.0.0 milestone Dec 16, 2019
@jlamim
Copy link
Contributor

jlamim commented Dec 16, 2019

I think it's important to have this information in the documentation so that everyone who is already familiar with this new version of the framework can know about this issue related to how the IsAJAX () method works when used with pure JavaScript.

If you wish I can organize the information added here in this issue and insert this information into the IncomingRequest class documentation.

@lonnieezell
Copy link
Member

@jlamim that would be great. I think we need the information on its own page, since it seems it would be referenced from a few different places, including Controller Filters, and Routing maybe?

@jlamim
Copy link
Contributor

jlamim commented Dec 16, 2019

@lonnieezell when you said "own page" did you mean that it would be better to post to my personal page / blog or the respective method page within the documentation?

If it's a specific page of the documentation, which page do you recommend?

@lonnieezell
Copy link
Member

@jlamim I mean we should create a new page in the documentation specifically about AJAX calls that can be referenced from the 3-4 places it should be mentioned.

@jlamim
Copy link
Contributor

jlamim commented Dec 16, 2019

Now it is clear to me. I think it is very valid for us to adopt this strategy regarding AJAX content with CodeIgniter 4. It will make it much easier for the community, as this feature tends to be widely used now that it is easier to integrate CodeIgniter with Vue, Angular and React.

@jlamim
Copy link
Contributor

jlamim commented Dec 16, 2019

@lonnieezell and @MGatner what do you think about creating a new topic in the Tutorials section, with this information and example, until we can find an efficient and easy-to-understand way to address the issue within the framework?

@najdanovicivan
Copy link
Contributor

@lonnieezell and @MGatner what do you thing about adding config for is_ajax so that it is possible to set which headers should be picked up to state that request is ajax something like this

$ajax_headers = [
    ["X-Requested-With" => ""XMLHttpRequest"],
    [
        "Content-Type" => "application/json",
        "X-Requested-With" => "JSONHttpRequest",
    ],
];

With this sample config requests which contain "X-Requested-With": "XMLHttpRequest" or both "Content-Type" : "application/json" and "X-Requested-With" : "JSONHttpRequest" will be considered as ajax requests.

@MGatner
Copy link
Member Author

MGatner commented Jan 26, 2020

The problem is that fetch requests send absolutely no distinguishing headers over regular browser requests (unless the developer chooses to add them). I think isAjax() is becoming a dated function and endpoints that need to differentiate request behavior will have to specify that as part of their definition.

@MGatner
Copy link
Member Author

MGatner commented Jan 30, 2020

FWIW Laravel's Request object has a function ajax() that is actually just a wrapper for Symfony's isXmlHttpRequest(). Both a simple checks against the X-Requested-With header and will fail to the same scenarios as ours.

Ref. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L1707

@lonnieezell
Copy link
Member

Makes sense. So many people use Javascript frameworks that automatically insert that so it doesn't get as noticed, I don't think. But without more pure javascript usage and especially raw fetch, on the rise, it will be more problematic. I think the best we can do is document it.

@jlamim
Copy link
Contributor

jlamim commented Jan 30, 2020

I agree with @lonnieezell . With so many variations, the best we have to do at the moment is to define a way of working with the IsAJAX () method that works for the largest possible number of requests and in the official documentation we put more details on these differences in relation to the X-Requested-With. Then we follow and see the feedbacks and from them we think of other forms of implementation that make the method even more efficient.

@element-code
Copy link
Contributor

Sorry for digging in the Graves but I stumbled over the documented way to use this and thought it would be better to:

  • deprecate the whole isAJAX functionality
  • make content negotiation easier to use (i think thats the only reason to use the isAJAX function "it's easier")

This has the following advantages:

  • we get rid of (dirty) workarounds
  • we force the user to write better code without more effort

If deprecating the isAJAX functionality is not an option:

  • give the user a hint to prefer content negotiation over isAJAX in the documentation

@MGatner
Copy link
Member Author

MGatner commented Oct 5, 2020

@element-code I'm definitely open. I suspect that the current isAjax headers will only get less common, and moving toward a proper implementation is a good idea. Would you like to start a PR with some of the content negotiation handling you mentioned? There's a lot happening with HTTP/API components right now so it would be a good moment for a push in that area.

@MGatner
Copy link
Member Author

MGatner commented Oct 5, 2020

Pulling in @caswell-cs in case of overlap

@element-code
Copy link
Contributor

element-code commented Oct 8, 2020

What also needs to be thought of is if there are other usages of the isAJAX functionality.

  • content negotiation
  • determining if the full page should get rendered or only the content (ajax-loading-content) -> any ideas how to detect this / which headers to use
  • use in filter application (e.g. cli)

Is there any other usage for isAJAX which we should add to this list?


Content negotiation as is seems pretty easy to use, maybe we should implement enums/constants with the "classic" web content types. Creating an alias isJSONin the request which internally uses content negotiation might make it even easier to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

5 participants