[5.5] Fix expectsJson returning true when expecting html

[jn-jairo]

The function expectsJson was returning true when not expecting JSON but expecting HTML.

I got this error in the return of a validation, it always returns a JSON, even when I put a dataType: 'html'.

Example:

$.ajax({
    method: 'POST',
    url: 'foo/bar',
    data: {foo: 'bar'},
    dataType: 'html'
});

I hope this correction is satisfactory, I could not think of a more elegant way to solve the problem without risking breaking someone's code.

[laurencei]

Looking at the wantsJson() code - I dont understand why it is triggering on your ajax example? It should only trigger if you ask for json? Where is that in your ajax request?

Would be nice if we can get a test demonstrating where the bug is if possible

[sisve]

dataType: 'html' sends an Accept:text/html header which wantsJson shouldn't consider json-ish.

This is most probably an error in your code somewhere. Show the request headers your ajax call is generating, and at least a failing test for wantsJson. If you're right we want to fix this with a test that verifies that the fix is never forgotten/reversed.

[jn-jairo]

The current code is:

    public function expectsJson()
    {
        return ($this->ajax() && ! $this->pjax()) || $this->wantsJson();
    }

So in my code the expectsJson is returning true because ajax() is true and pjax() is false

[sisve]

You're right; I was only thinking of the Accept header. The comment even says "Determine if the current request probably expects a JSON response", my emphasis.

Changing what expectsJson does in a released version would be a breaking change. I would suggest that you target the 5.6 release (the master branch) and add some tests to make sure your new functionality stays in place and aren't accidentally refactored away.

[crynobone]

Might be better to use $request->prefers('text/html'); insteads of wantsJson() or expectJson().

[taylorotwell]

Is there a reason you didn't use $request->accepts() when determining if the request wants HTML?

[jn-jairo]

Thanks @sisve, I was really worried that changing this could cause problems. I'm busy now, but I should be able to work on it from the 21st, and I'll add some tests.

[jn-jairo]

@taylorotwell in my code I can use $request->accepts(), but when a validation fails the default return uses $request->expectsJson() in Illuminate\Foundation\Exceptions\Handler::convertValidationExceptionToResponse(), is there any way I can overwrite it?

[jn-jairo]

I've found where I can override the convertValidationExceptionToResponse method in my code and swap expectsJson with another method, I'll use that for now.

If someone has an idea how to fix this without a breaking change I make the change, otherwise I think it will be better to close this pull request and add a better solution in version 5.6.

Thank you all for your help.

[taylorotwell]

Is this really a breaking change or is it a bug fix? We are literally telling the backend we want HTML and Laravel is saying "oh this request expects JSON!"... that's a bug.

[sisve]

expectsJson() says that the request probably wants json when a request has X-Requested-With: XMLHttpRequest, no matter the actual Accept-header. The wantsJson() is properly checking the Accept-header.

The assumption that all ajax calls expects a json response is weird. I consider this both a bug, and a huge breaking change. We have no idea how expectsJson() is used in the wild, and how many sites will break if we change it now.

[jn-jairo]

What do you think of this solution?

    public function wantsAnyType()
    {
        $acceptable = $this->getAcceptableContentTypes();

        return isset($acceptable[0]) && ($acceptable[0] === '*/*' || $acceptable[0] === '*');
    }

    public function expectsJson()
    {
        return ($this->ajax() && ! $this->pjax() && $this->wantsAnyType()) || $this->wantsJson();
    }

If the request is ajax and is not pjax and the expected Accept is */* or *, then we assume that the expected return is JSON.

Then:
If Accept: */* expectsJson returns true
If Accept: application/json expectsJson returns true
If Accept: text/html expectsJson returns false
If Accept: text/html, */* expectsJson returns false
If Accept: application/json, text/html expectsJson returns true
If Accept: text/html, application/json expectsJson returns false

Do you think this solves the bug without adding a breaking change?

[sisve]

Any change to expectsJson() is a breaking change. I think it should target 5.6 (the master branch) where breaking changes are allowed.

[GrahamCampbell]

I think you are confusing this with acceptsJson?

[pavinthan]

@jn-jairo It's should return JSON response ( as expected ) because, your request is ajax

public function expectsJson()
{
   return ($this->ajax() && ! $this->pjax()) || $this->wantsJson();
}

here is wantsJson check Accept headers

[sisve]

@pavinthan He's telling jQuery to expect a html result, and he sends a Accept: text/html header. The wantsJson() check returns false. It's the ajax() check that presumes that everything that is ajax wants json, which isn't correct in this case.

[pavinthan]

That correct, small suggestion why this may change like

public function expectsJson()
{
   return $this->wantsJson();
}

if someones need the JSON response, they should have Accept header
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.1

[sisve]

I still think that this should target the 5.6 release (the master branch).

[pavinthan]

Guys please review laravel/ideas#933

[jn-jairo]

I opened a pull request targeting master (#22506) and also applied the change in this pull request.

I'll let you decide which branch to apply this to.