DAV client: Check if array itself has property in for loop

[leonklingele-work]

This prevents looping through properties of the array's prototype

[leonklingele-work]

@PVince81 @evert ptal

[PVince81]

@leonklingele-work did you test this with IE11 ? Would be good to also test with latest Safari, etc to make sure they all work properly with this method call.

[leonklingele-work]

@PVince81 no, but it should be supported by every browser. Even IE6 supports it and it has been in the standards since.

Steps to reproduce the issue I have:

1. Make sure this patch is not applied
2. Go to your 'Personal Settings'
3. Open the developer console an paste this (extends all arrays with a new diff method):

Array.prototype.diff = function(a) {
	return this.filter(function(i) {
		return a.indexOf(i) < 0;
	});
};

4. Try to change your profile picture by opening the file picker to select a file
5. File picker doesn't open, Uncaught TypeError: propertyName.match is not a function [..]

[PVince81]

@leonklingele-work can you test it at least with IE11 then ? That's the one browser I usually don't trust much with "shoulds" 😉

https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/

[PVince81]

Raised owncloud/core#28971 to upgrade the lib in ownCloud once this PR is merged. I wasn't aware that it would break OC features. Thanks.

[leonklingele-work]

The fix works fine in Safari 10 and IE11.
I have updated this PR to run the check in ever for .. in properties loop (we have two such loops).
Please take another look.

[PVince81]

Now thinking of it, would you mind adding a unit test for this ?

Let me know if you need help.

I suggest simply adding a global beforeEach on which you add a custom property on the Array, and then remove it again in afterEach. This would run all possible tests with the extra property and confirm that your fix helps. (try running without your fix to confirm that the problem is detected)

[leonklingele-work]

Sorry — unit tests are unrelated to this change. They should be provided in a separate PR.

[PVince81]

unit tests are unrelated to this change

In general every PR should come with unit tests to validate that the change that was made is covered by tests. If you don't want to write unit tests I'm fine doing this myself, just let me know.

[PVince81]

As discussed on IRC, I'll take care of this. Thanks!

[PVince81]

pushed here: b375917

I verified that the test actually works by commenting out the changes in this PR, and the tests indeed failed.