Lots of behaviors, like KeyboardNavigation, etc., have 'invisible dependencies' on components of OpenLayers.js which are not necessarily part of every OpenLayers.js build. These can break the site, because the code will call OpenLayers.Control.* and javascript knows that it's a no-method-found problem. This problem is mildly caught by the fact that attachBehaviors is surrounded by a try catch block, but we need to do more:

  • We may want to surround each behavior / the parent of each behavior with another try catch block so that individual ones can fail
  • We may want to keep track of what openlayers components are required by specifying them in behaviors + layers
  • There is the third, but untasty option of including the necessary OpenLayers core code with behaviors. This has many drawbacks, among them the fact that this is slow for Drupal to compile, requires mixing of GPL and BSD code, and will be unnecessary for the majority of users.
CommentFileSizeAuthor
#12 dependency-detection-3.patch25.92 KBtmcw
#7 dependency-detection.patch2.6 KBtmcw

Comments

zzolo’s picture

zzolo commented

This makes sense as we want users to be able to use a streamlined library. I definitely agree that including OL code in the module is not a good idea.

I think the key is communication to the user. Most people might get confused by this, and so its important to tell the user that this behavior requires a specific Control or whatever. Error handling is important, but if everything works fine and the user is not told anything, then the user is just without a behavior but no clue why.

With good error handling, I think it would make sense to extend the base behavior object to include a way of storing requirements for the behavior. This will make it easy to tell the user about the requirements, and opens up the possibility of the module providing an automatic or semi-automatic way of compiling a OL library (should we want to do that).

tmcw’s picture

tmcw commented

Error handling is important, but if everything works fine and the user is not told anything, then the user is just without a behavior but no clue why.

Definitely. One possibility that I was thinking about was to output critical error messages (which prevent the map from rendering) to the map div, and to output behavior errors in a panel on the map, so that it's easier for users to debug.

And, yeah, behaviors and layer types would be smart to provide OpenLayers requirements; it's going to be tricky or impossible to detect what the particular OpenLayers.js file includes to give prescriptive warning in the admin.

zzolo’s picture

zzolo commented

Still thinking about all this, but one quick idea I had was to create an Analyze page. Instead of trying to offer real-time debugging and feedback, we make a new page that analyzes everything and gives feedback based on that. This would allow better analysis of the OpenLayers library without sacrificing performance.

About errors, I don't think outputting errors to the screen without explicit permission is a good idea. This is what the log is used for, even though most people don't actually check it.

tmcw’s picture

tmcw commented

Note that as far as 'outputting errors' I mean, exclusively, javascript errors, via Javascript exceptions. This what the js currently almost does (although it logs to console.log if it is defined). I really don't think that there's much problem with this, because there's very little security risk and 'the log' is not a real option when dealing with javascript. Maybe you thought I meant PHP errors - those will obviously be pushed into watchdog.

I don't see much reason to do an analyze page... it seems like the kind of analysis that that would do would be essentially what we need as a validate handler on preset saving, and there's absolutely no performance risk to running a small amount of logic there. Having the module ensure that input is valid on input - not only, say, not-blank or a number or alphanumeric, but reasonably valid as in this-map-will-work, I think, is much better than adding another feature whose place in the building-a-map workflow is not obvious.

zzolo’s picture

zzolo commented

What about this:

* Extend behavior object to be able to specific what OpenLayers object it needs.

This can then be used for error catching and handling on the behavior side, without having to manually encompass all behaviors in a catch block. And it means that we can begin to tell the user about what OpenLayers parts are need (probably in the list of behaviors).

tmcw’s picture

tmcw commented

While writing the MultiMap layer type, I got this random idea:

How about, on certain admin pages, we call openlayer_include() and then have a small chunk of jQuery which simply outputs something like a green box with OK, or a red one with 'Not Supported' and a link to documentation which explains. This would be quite simple and could be useful - not sure exactly how it would play out with formapi, but I think it can be accomplished with a minimal amount of code and would be very effective / not prone to terrible edge-cases.

tmcw’s picture

tmcw commented
StatusFileSize
new dependency-detection.patch2.6 KB

A first thwack at this problem; there's a bit more to do (there should be a helper function for the form element, it shouldn't use eval, styling of the indicator could be better), but this gives you the idea of what's going down.

zzolo’s picture

zzolo commented
Status: Active » Needs work

Seems like a light, simple mechanism. Some issues:

* I would rather see sending JSON to the JS side then putting HTML into a form item. It just seems much cleaner than using HTML and a non-standard HTML attribute.
* I would also rather see this dependency be put into the behavior class definition as a property or within a specific method. It just makes much more sense to me if the interface handles all the logic of how to handle the dependency (instead of providing specific HTML). Also, this would allow logic in other places to deal with dependencies.
* CSS classes usually use dashes.

tmcw’s picture

tmcw commented

* I would rather see sending JSON to the JS side then putting HTML into a form item. It just seems much cleaner than using HTML and a non-standard HTML attribute.

Not quite sure about this. How is this better? How could an implementation be simpler? I'm not hugely against the idea but feel like this would add a significant layer of complexity on top of this and for very little win. If this were to be renamed so that it's a valid data attribute would that work?

I agree as far as putting it in a different part of the behaviors class, and renaming the classes.

zzolo’s picture

zzolo commented

Admittedly, this is a nicely, simple solution. But:

* Using data attributes and non specified attributes means that in XHTML strict and some other standards, the markup will not validate (and technically the Drupal standard is XHTML 1.0 Strict).
* Using eval() is not a good idea.
* Having to write that amount of code (and in the settings form) to specify a dependency is not very natural.
* Using HTML markup in form element is not reusable if there was a need to use the dependency designation somewhere else.

I would rather see a dependency parameter on the behavior (or layer) definition that can be processed in many different ways if needed. It would be a little more code, but much more portable solution.

But, I also dont have a patch to back up my mouth. :)

tmcw’s picture

tmcw commented

Uh, yes, all of those points except for the fourth I noted in #7 when I posted the patch.

tmcw’s picture

tmcw commented
StatusFileSize
new dependency-detection-3.patch25.92 KB

An updated version of the patch which addresses those problems.

tmcw’s picture

tmcw commented
Status: Needs work » Fixed

Patch committed: http://drupal.org/cvs?commit=368606

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.