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

FIX Don't overwrite JSON errors with templated error page #1334

Merged

Conversation

GuySartorelli
Copy link
Member

The new error handling logic was overwriting JSON error messages, which are most likely responses for AJAX requests. A more complete fix would be to find out why they weren't being detected as AJAX requests and resolve that - but that could lead to even more regressions, and would be time consuming besides.

Parent issue

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems appropriate as we use the "hardcoded" string 'application/json' elsewhere in LeftAndMain.js to denote XHR requests

HTTPRequest::isAjax() looks like a sub-optimal method as it requires setting querystring or header values client side, instead of a smarter automatic detection, so likely it's giving the wrong result in many instances

HTTPRequest.php

    public function isAjax()
    {
        return (
            $this->requestVar('ajax') ||
            $this->getHeader('x-requested-with') === "XMLHttpRequest"
        );
    }

@emteknetnz emteknetnz merged commit 523821b into silverstripe:1 Aug 1, 2022
@emteknetnz emteknetnz deleted the pulls/1/fix-regression branch August 1, 2022 00:30
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.

Regressions caused by new error handling in LeftAndMain
2 participants