Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($httpBackend): fix HTTP PATCH requests in IE8 #5043

Closed
wants to merge 2 commits into from
Closed

fix($httpBackend): fix HTTP PATCH requests in IE8 #5043

wants to merge 2 commits into from

Conversation

jeffsmohan
Copy link

In Internet Explorer 8, the native XMLHttpRequest object does not allow the HTTP PATCH method. In order to accomplish a PATCH, you need to use the ActiveX object. Update the default XHR to use the ActiveX object when doing a PATCH in IE8. See the following for documentation:

Discussed previously in #2518. I wanted to put together an example of how we could fix this, and Travis reports a green light on this branch. I'm not too familiar with the structure and internals of the project yet, though, so if you point me in the direction of how to add tests for HttpBackend (which appears to be mocked in testing) and any cleanup needed, I'm happy to work on it.

@petebacondarwin
Copy link
Contributor

@jeffschenck - thanks for the PR. Have you signed the CLA? Also we need to be able to test this change. Can you knock up a unit test that highlights the problem in IE8 before your change fixes it?

@jeffsmohan
Copy link
Author

@petebacondarwin I have signed the CLA.

As I mentioned, I'm happy to work up some tests for this, but I'm not too familiar with the test suite setup yet. And this seems like a particularly special-case sort of situation — testing HttpBackend, a service that's mocked in most of the test suite; and testing it particularly in one browser, IE8. Can you point me in the right direction? What file would this test be added in? How do I look to see tests first failing and then passing in IE8?

Let me know and I'll work up the test(s). Thanks!

@petebacondarwin
Copy link
Contributor

@jeffschenck - Thanks for this. Regarding writing a test. This file would be a good place to start: https://github.com/angular/angular.js/blob/master/test/ng/httpBackendSpec.js

Then you should look at running the tests locally, grunt test.

@jeffsmohan
Copy link
Author

@petebacondarwin Thanks — forgive me if I've missed something obvious, but I've had the tests passing locally and looked through httpBackendSpec.js. The problem is that all the calls to createHttpBackend in that file use mocked versions of XHR, which won't expose the IE8 quirk that causes this bug. As far as I can figure, in order to reproduce the error in IE8 on Travis, we need to actually use the real XHR object and really attempt to make a request.

Is there somewhere outside of the httpBackendSpec.js unit tests (e2e testing?) where the actual XHR object is used and actual requests are attempted? Would this be a better place for this test? Or, alternatively, am I missing some way to reproduce the bug in IE8's version of the XMLHttpRequest object?

Thanks for the help!

@petebacondarwin
Copy link
Contributor

We do run our tests on IE8. I haven't look in detail right now but I assume that you are going to write a test that checks that if the test is on IE8 then the ActiveX object is being used instead of XHR...

Jeff Schenck added 2 commits December 12, 2013 15:14
… operations

In Internet Explorer 8, the native XMLHttpRequest object does not allow the
HTTP PATCH method. This tests that in IE8, when performing a PATCH, we fall
back to ActiveXObject.
In Internet Explorer 8, the native XMLHttpRequest object does not allow the
HTTP PATCH method. In order to accomplish a PATCH, you need to use the ActiveX
object. Update the default XHR to use the ActiveX object when doing a PATCH in
IE8. See the following for documentation:

http://blogs.msdn.com/b/ieinternals/archive/2009/07/23/the-ie8-native-xmlhttprequest-object.aspx
http://bugs.jquery.com/ticket/13240
@jeffsmohan
Copy link
Author

Updated the pull request with additionally a test for using ActiveXObject in IE8 PATCH requests. Let me know if there's anything else needed here, thanks!

@@ -1,5 +1,15 @@
'use strict';

// We need to be able to detect IE8 so we can use the right AJAX method below
Copy link
Contributor

Choose a reason for hiding this comment

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

none of this is needed. just use msie variable that we expose internally

Copy link
Author

Choose a reason for hiding this comment

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

Good to know. I had assumed it was only exposed in the test environment.

IgorMinar added a commit to IgorMinar/angular.js that referenced this pull request Dec 13, 2013
IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

Closes angular#2518
Closes angular#5043
@IgorMinar
Copy link
Contributor

I created an alternative fix as #5390.

thanks for the PR

@IgorMinar IgorMinar closed this Dec 13, 2013
wesleycho pushed a commit to wesleycho/angular.js that referenced this pull request Dec 31, 2013
IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

Closes angular#2518
Closes angular#5043

Fixed createXhr function to throw minErr when an exception occurs
wesleycho pushed a commit to wesleycho/angular.js that referenced this pull request Dec 31, 2013
IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

Closes angular#2518
Closes angular#5043
IgorMinar added a commit to IgorMinar/angular.js that referenced this pull request Jan 3, 2014
IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

I'm also removing the noxhr error doc because nobody will ever get that error.

Closes angular#2518
Closes angular#5043
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

I'm also removing the noxhr error doc because nobody will ever get that error.

Closes angular#2518
Closes angular#5043
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

I'm also removing the noxhr error doc because nobody will ever get that error.

Closes angular#2518
Closes angular#5043
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants