OpenLayers OpenLayers

Ticket #1502 (reopened feature)

Opened 3 months ago

Last modified 3 months ago

Events register method fails if listeners member is not an array

Reported by: pgiraud Assigned to: euzuro
Priority: minor Milestone: 2.7 Release
Component: Events Version: 2.6 RC1
Keywords: Cc:
State: Needs More Work

Description

If Array.prototype is extended by a javascript code outside OpenLayers, and is been added a function as new member, the register method in Events.js fails when trying to push something in this "new" member. The patch to follow tests if the member is an Array.

Attachments

patch-1502-r6825-A0.diff (0.5 kB) - added by pgiraud on 04/08/08 11:48:57.
object_extension.patch (1.6 kB) - added by crschmidt on 04/09/08 09:48:36.
object_extension.2.patch (1.6 kB) - added by pgiraud on 04/09/08 10:36:51.
same as chris' patch but "m" replaced by "map"
events.patch (1.7 kB) - added by crschmidt on 04/09/08 11:34:59.

Change History

04/08/08 11:48:57 changed by pgiraud

  • attachment patch-1502-r6825-A0.diff added.

04/09/08 03:47:21 changed by pgiraud

crschmidt wrote on IRC :

I think that a hasOwnProperty check in the 'un' function might actually be a better way to go here a more comprehensive look at the various places we loop over an object like that might make sense as well for example, HttpRequest does 'for (var key in allParams)'

04/09/08 03:48:26 changed by pgiraud

(same with punctuation for readability)

I think that a hasOwnProperty check in the 'un' function might actually be a better way to go here.

A more comprehensive look at the various places we loop over an object like that might make sense as well.

For example, HttpRequest does 'for (var key in allParams)'.

04/09/08 08:29:00 changed by crschmidt

  • milestone set to 2.6 Release.

I think that fixing the 'Events' thing is a blocker for 2.6: in 2.5, you could do Object.prototype.foo=function() {return 'bar'}; and simple examples still worked, but in 2.6 you can't.

Doing the more general sweep can wait, so long as we don't actively throw an error, as far as I'm concerned.

04/09/08 09:48:36 changed by crschmidt

  • attachment object_extension.patch added.

04/09/08 09:49:28 changed by crschmidt

  • state set to Review.

Under Tim's advisement, I've patched Events.js to only register a type of it is in the valid events list, thus protecting ourselves, and added an 'extras.html' to test this 'functionality'.

04/09/08 10:36:51 changed by pgiraud

  • attachment object_extension.2.patch added.

same as chris' patch but "m" replaced by "map"

04/09/08 10:42:10 changed by pgiraud

  • state changed from Review to Commit.

The patch looks good, I think it's ready for commit.

04/09/08 11:26:00 changed by crschmidt

  • state changed from Commit to Needs More Work.

IE Popup tests fail with this patch.

04/09/08 11:34:59 changed by crschmidt

  • attachment events.patch added.

04/09/08 11:56:37 changed by tschaub

  • state changed from Needs More Work to Commit.

Looks good to me (without the sniff). Assuming all tests pass everywhere, please commit.

04/09/08 12:01:23 changed by tschaub

Please commit events.patch without the sniff.

04/09/08 12:06:23 changed by crschmidt

  • keywords set to pullup.
  • state changed from Commit to Pullup.

(In [6832]) 2.6 introduced a regression that maps could not be created when the Object prototype is extended. Although OpenLayers is not designed to work when this fundamental violation of Javascript is in place, we can at least not fail so miserably in this case: this gets us back to a similar level of functionality as we had in 2.5. This was brought up by a user working on a viamichelin layer, which apparently extends the object prototype (bad!), and fixed in conjuction with tschaub. r=pgiraud,tschaub (Pullup #1502)

04/09/08 14:00:21 changed by crschmidt

  • status changed from new to closed.
  • state changed from Pullup to Complete.
  • resolution set to fixed.

(In r6835) Finish pullups for RC2.

#1498 Easily turning off/overriding default select and temporary intent styles #1501 GeoRSS format tests fail in Safari #1502 Events register method fails if listeners member is not an array #1503 panning off for odd-sized viewport #1504 doc review

04/18/08 10:51:58 changed by crschmidt

  • status changed from closed to reopened.
  • state changed from Complete to Needs More Work.
  • resolution deleted.
  • milestone changed from 2.6 Release to 2.7 Release.

http://openlayers.org/pipermail/users/2008-April/005676.html

addEventType doesn't add the event to the EVENT_TYPES list on the events object, so addEventType as an API method doesn't successfully allow for the registration of the events.

The only place this is used outside the Events class is in Layer.Grid.

Adding this logic to addEventType may not be the right way to go, because addEventType is used in the Events constructor when looping through the EVENT_TYPES. It might be better to have another API method to do this -- or to move addEventType to prepareEventListeners, and use addEventType as the external API which calls both prepareEventListeners *and* adds to EVENT_TYPES.

04/25/08 08:41:37 changed by crschmidt

  • keywords deleted.