Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In #995854-58: #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form, both sun (#64) and I noticed in testing a situation in Drupal.ajaxError where the xmlhttp.statusText or xmlhttp.responseText were not populated in the object. Therefore, as Drupal.ajaxError attempted to process them, it created an unhandled exception itself.
This patch resolves that error path, which could be a particularly nasty one, especially since this extended error checking was explicitly put here to help us *track down* "AJAX statusCode = 0" problems.
How to recreate the original problem:
- Apply #995854-58: #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form
- Install Examples Project
- Enable AJAX Example
- Go to examples/ajax_example/submit_driven_ajax and press the submit button
- You will notice that the page submits instead of an AJAX submit happening. If you look in firebug, you'll see an aborted POST response.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | drupal.ajax_error_handling_05.patch | 1.48 KB | rfay |
| drupal.ajax_error_handling_00.patch | 1.48 KB | rfay | |
Comments
Comment #1
rfayComment #2
effulgentsia CreditAttribution: effulgentsia commentedsubscribe. haven't thought about this enough yet to comment. curious what sun thinks.
Comment #3
rfayJust FYI, the reason for the two separate try/catch blocks is that we might conceivably have info in one of the object members but not in the other.
Comment #4
tstoecklerMinor: Trailing whitespace.
Powered by Dreditor.
Comment #5
rfayThis removes the trailing whitespace.
I discovered that I had eclipse configured (forever) to remove trailing whitespace on PHP, but it wasn't doing it for JS. So I will annoy sun less in the future :-)
I would *really* like to get this in before launch, because it potentially may affect our ability to resolve AJAX problems when they occur. I'd love for D7 to have 50% fewer "AJAX Error 0 has occurred" issue filings :-) And for the ones that get filed to actually be understandable.
Comment #6
aspilicious CreditAttribution: aspilicious commentedI used my premature js/programmer skills to give this a review.
This code *only* puts a try catch block around responseText and statusText.
Without this patch
- if statusText or responseText throw an error, we will leave this function till we find a catch
With this patch
- if responseText throws an exception, it will be catched emediatly and we still can fetch the statustext. Which has more usefull information than the previous error.
The only concern I had was that statusText and responseText may not be initialised if they throw an error. But rfay initialises them before the try catch blocks.
I can't think of any way this could break something.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedThis could probably be bikeshed a bit, and it would be nice to understand a little more clearly why testing for statusText or responseText throws an exception, but such bikeshedding and further probing can happen after 7.0 is released. I agree with rfay that catching these remaining exceptions and alerting them (as we do for all other AJAX error paths) is better than letting them happen silently, or get alerted by the browser as some cryptic "AJAX Error 0 has occurred" message. And that it's better to get this in before 7.0 is released, so that better issue reports get filed when something does go wrong.
Comment #8
Kiphaas7 CreditAttribution: Kiphaas7 commentedIs it still possible to have a reliable test case? I tried the steps in the OP, but wasn't able to get the error. I did get the race condition if I double-clicked the button, making a regular form submit, but didn't get the aborted POST in firebug.
* Checked out a fresh HEAD install, so didn't apply because #995854 got committed (though the patch mentioned in the OP is #58 and that isn't the exact patch that got committed.
* Installed Examples Project
* Enable AJAX Example
* Went to examples/ajax_example/submit_driven_ajax and press the submit button
* "You will notice that the page submits instead of an AJAX submit happening. If you look in firebug, you'll see an aborted POST response.". No it didn't, it just made a successful AJAX submit, only if I clicked while it was doing an AJAX POST, it would do a regular POST, but I believe that is a different bug.
Comment #9
rfay@Kiphaas7, you could do it with the exact procedure above in the initial report, but you'd have to use an older commit level (before the actual commit that went in for that issue). However, that's not really the point. The point is that there is a way to get an exception that causes total loss of information (and interrupts js execution).
Thanks,
-Randy
Comment #10
webchickThis seems like a pretty solid robustifying of error handling here.
Committed to HEAD. Thanks!