XHR requests should get JSON for fatal errors #6228

Closed
mrclay opened this Issue Nov 19, 2013 · 20 comments

Comments

Projects
None yet
3 participants
@mrclay
Member

mrclay commented Nov 19, 2013

E.g. use elgg.action() for an ajax action that has a fatal error. The response is the typical HTML page, so nothing is shown to the user.

@Srokap

This comment has been minimized.

Show comment
Hide comment
@Srokap

Srokap Nov 19, 2013

Contributor

👍 I believe we even have success flag in the response already?

Contributor

Srokap commented Nov 19, 2013

👍 I believe we even have success flag in the response already?

@jdalsem jdalsem self-assigned this Dec 2, 2014

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 4, 2015

Member

this is impossible to do as we do not standardize results from actions. If someone chooses to exit in an delete action for example catching this is impossible. Also fatal errors do not result in correct error codes to be caught by an ajax error handler.

See also #5134

Member

jdalsem commented Feb 4, 2015

this is impossible to do as we do not standardize results from actions. If someone chooses to exit in an delete action for example catching this is impossible. Also fatal errors do not result in correct error codes to be caught by an ajax error handler.

See also #5134

@jdalsem jdalsem closed this Feb 4, 2015

@mrclay mrclay reopened this Feb 4, 2015

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 4, 2015

Member

what is your idea @mrclay ?

Member

jdalsem commented Feb 4, 2015

what is your idea @mrclay ?

@jdalsem jdalsem removed their assignment Feb 4, 2015

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

I'm suggesting to adjust the exception handler to detect XHR, and if so, maybe send a simple {"exception":"..."} that would be caught by elgg.action()

Member

mrclay commented Feb 4, 2015

I'm suggesting to adjust the exception handler to detect XHR, and if so, maybe send a simple {"exception":"..."} that would be caught by elgg.action()

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 4, 2015

Member

impossible as ajax calls could do a exit

Member

jdalsem commented Feb 4, 2015

impossible as ajax calls could do a exit

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

If they successfully get to the exit(), then there's no problem to report.

Member

mrclay commented Feb 4, 2015

If they successfully get to the exit(), then there's no problem to report.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 4, 2015

Member

oh i was too quick on the trigger, but that would means it can only work with an exception handler... still fatal errors will not be caught...

Member

jdalsem commented Feb 4, 2015

oh i was too quick on the trigger, but that would means it can only work with an exception handler... still fatal errors will not be caught...

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

Yes, but that's PHP's (current) limitation. There are RFCs to make more exceptions catchable.

Member

mrclay commented Feb 4, 2015

Yes, but that's PHP's (current) limitation. There are RFCs to make more exceptions catchable.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

The most common exception I hear of is calling methods on null/false (e.g. assuming get_entity() returned an entity). If that can't be handled, then there's probably no point to this.

Member

mrclay commented Feb 4, 2015

The most common exception I hear of is calling methods on null/false (e.g. assuming get_entity() returned an entity). If that can't be handled, then there's probably no point to this.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 4, 2015

Member

okay, so the exception handler detects a elgg_is_xhr() and then returns an ErrorResult with the exception message as the error message (register_error)?

Member

jdalsem commented Feb 4, 2015

okay, so the exception handler detects a elgg_is_xhr() and then returns an ErrorResult with the exception message as the error message (register_error)?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

I'm thinking the exception handler would detect elgg_is_xhr(), and if so just send a JSON response and exit.

Member

mrclay commented Feb 4, 2015

I'm thinking the exception handler would detect elgg_is_xhr(), and if so just send a JSON response and exit.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 4, 2015

Member

why not a ErrorResult? that will already be caught by elgg.action

Member

jdalsem commented Feb 4, 2015

why not a ErrorResult? that will already be caught by elgg.action

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

OK, yes. I didn't know ErrorResult was an established response type.

Member

mrclay commented Feb 4, 2015

OK, yes. I didn't know ErrorResult was an established response type.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

Where is this handling code in js/lib?

Member

mrclay commented Feb 4, 2015

Where is this handling code in js/lib?

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

Thx. I'm thinking in these cases elgg.action should not continue to call the custom_success handler. Effectively, this response means we can't guarantee anything on the server happened. It should probably trigger the error handler you've added here: #7853

Member

mrclay commented Feb 4, 2015

Thx. I'm thinking in these cases elgg.action should not continue to call the custom_success handler. Effectively, this response means we can't guarantee anything on the server happened. It should probably trigger the error handler you've added here: #7853

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

...but allow the exception message to be shown to the user, not just the generic language.

Member

mrclay commented Feb 4, 2015

...but allow the exception message to be shown to the user, not just the generic language.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 4, 2015

Member

if the exception is having a 404 header the generic function (from #7853) is already catching it i think. We could still check the body for valid json and check for the exception text being available like you suggested.

Member

jdalsem commented Feb 4, 2015

if the exception is having a 404 header the generic function (from #7853) is already catching it i think. We could still check the body for valid json and check for the exception text being available like you suggested.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 4, 2015

Member

Right, I think we'd be sending a 5xx response code with the JSON.

Member

mrclay commented Feb 4, 2015

Right, I think we'd be sending a 5xx response code with the JSON.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue May 25, 2015

fix(http): More appropriate exception responses
Uncaught exception responses now have a 500 HTTP code, and XHR requests
are given a JSON response.

Fixes #6228
Fixes #8360
@mrclay

This comment has been minimized.

Show comment
Hide comment
Member

mrclay commented May 25, 2015

PR #8362

@jdalsem jdalsem closed this May 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment