More in-depth/sane structure checking is needed for map rendering

I would kind of go the opposite direction of more automation in making map arrays. I think that the code should basically require its implementors - openlayers_ui, openlayers_cck, other modules - to follow a much stricter set of requirements. It just doesn't do us any good to have a map array that could have so many different forms. Even if openlayers.js there's a switch for some random 'other form' of map options that I don't know where it comes from. So, less complexity, more strictness is my motto here.

I feel like we should make assumptions about what data is coming into our public API functions. Filling in empty/bad values just makes it harder to develop stuff for this module and makes future compatible much harder because people are throwing a variety of stuff at these functions. They are APIs, therefore there are restrictions with how they are used. It isn't a condition of an API that it never throws errors, or puts up with bad implementors.

Must've missed Programming 101 in undergrad CS :)

Right now, someone could easy do this: openlayers_build_map(123); and the function still makes the assumption that that data is an array, let alone that there are specific parts in that array.

No, they cannot. Try it yourself.

But we currently don't have robust checking on data coming into the map render function and therefore bad data gets put through the rendering process.

Another no. If the function assumes that a type is an array, and it calls an index (see example above), then PHP will report that you're 'doing it wrong.' It is never a good idea to implement some sort of weird strong-typing type-checking system in PHP when PHP has its own, and it is an even worse idea to do this so that people who are implementing things using this module (developers) do not need to know its API. The API is documented, and comments reveal correct values and their types. If you do it wrong, then you get the same result as when you call any other function in PHP with the wrong kind of argument - a PHP error, not a Drupal error.

Apparently this is turning into another big ol' argument, so I'll just put it on the backburner for now and focus on other things. Not sure what your point really is or how node_load supports it.

Sorry, but that error seems massively straightforward to me: it's telling you that you need to use an array.

node_load supports multiple types for its arguments and handles those types. That does not mean that it will eat anything and guarantee that everything will be just fine. Same goes for other functions that support multiple types. I think that somehow this is getting smeared into a really bad idea of making PHP strongly typed within this module (it ain't) or guaranteeing that the module should not throw type errors when people give it incorrect types. Perhaps you're confusing this system with C++ or making assumptions about the API that I am not; nevertheless, I sadly must admit that I am not going to change my opinion on this issue. Once again, it is absurd to implement strong typing in PHP, and it is equally absurd to replace PHP's type checking system with something inside this module. You might want to talk to drupal people about this, or look for more examples, but there's a serious misunderstanding there.

I could continue to argue about this, but I'm not going to. I'm going to write patches and apply them on this ticket. If you want to give examples, citations, etc., of your opinion, please do.

Also: if you're looking for strict typing, let's just do it, instead of faking it within the function body. However, this kills the nature of PHP - for instance, the following definition of openlayers_build_map says that the argument needs to be a array, but the body of the function only needs $map to act like an array.

function openlayers_build_map(array $map = array()) {

Here's a patch that starts this process. This identifies the main need for some of the Javascript oddness going on in openlayers.js, and narrows down behavior a bit. I think that the map['errors'] structure is a bad vestigial practice (we should use exceptions, and not tolerate errors), but that'll come later. I'd appreciate testing on this, because it is a rather sweeping fix.

Er, I think we're moving in different directions here:

What I have are basically opinions, not divine commands, so I'll state them as such.

• I don't like the $error array. If there's an error that will prevent a map from working, then it is an error, and should be a drupal error, not in some other form. Half-working maps that the $error[] array allows will only bring more bugs.
• I don't like functions named 'process'. If they're hooks, then that's fine - hooks can do whatever they want, and indeed that does boil down to processing or altering things. But within this module, functions do things that are more specific than processing things, and the processing functions that used to be in the module only muddied the waters of comprehension of what was going on.
• Also adding a function called _openlayers_preprocess_map when one called openlayers_map_preprocess is already in the module is not a good idea
• I still don't get what is a map array and why it's good. Is this a preset? I feel like there's nothing I can point to which is both clean code and uses a 'map array' as a vector to creating maps here.

What my patch was aiming at is removing the if-else hooks that have accumulated through the months - the biggest and most pernicious one is the if-else in openlayers.js, which currently only serves to make the poorly-formed defaults preset work - its definition of projections is behind the times. I'd like to remove as much code of that sort as possible, so that there's a much better-defined idea of what exactly the module accepts and what it doesn't - this will make the internal code much more testable and stable.

Relative to the whole processing thing: I've realized that my aversion is basically because these processing functions are identical in function and specificity to 'goto'. And the fact that now they're turning into pass-by-reference shacks means that they aren't even referentially transparent. It's just a really bad thing.

If a map array is what a preset contains, then it is a preset - or a preset, right?

And, yes, I think that documentation should be first here, not 'preventing PHP errors from cropping up.' Preventing these errors from cropping up in this way - by silently defaulting their values - is just creating more confusing errors -or even worse - success despite incorrect use of the API. This is nothing but a bad thing, because then the API may change, and has a bunch of users counting on it to be something that it isn't, so there's no easy upgrade path. Like, the problem that I'm addressing in the javascript if-else statement is that the OpenLayers CCK module was doing the wrong thing, but the module and its javascript was putting up with it.

If we do, we need some mechanism to tell the user/program of the API about what went wrong and, IMO, in a way that is not just triggering a PHP error so that the user/progam of the API can handle these errors in its own way.

Okay: exceptions.

The whole structure of the rendering pipeline is confusing and hides important details and functionality far down in the stack. Those process functions really obscure things. IMO, while it may be necessary to process a map, passing in a structure that is called a map but that actually is an elaborate array that gets parsed and the ACTUAL data extracted and shoved back in. This is part of what I was grousing about in the issue I just created (http://drupal.org/node/805910). Shouldn't openlayers_render consume "pre-built" map data directly and simply render it?

Why should the rendering function care about presets or call a bunch of processing functions I don't expect it to? If I'm using OpenLayers as a developer, interacting with the OL module API, I should think the simplest approach is best: it should be up to me to create the appropriate data structure and provide data, and up to the module to render it...

Hi David,

Thanks for the input, but this ticket is about a different topic - let's keep discussion of a high-level rethink in the other ticket. I've responded to your points about render methods and why data isn't in the map array in the other ticket. That said, obviously I agree with the point that generic 'process' functions are a harmful addition to the module, and feel like that they should be entirely eliminated outside of hook-based usage.

Huh? Why close this when it has a patch on it that you want to be committed?

... I'll reroll this ticket and commit it soon.

Fixed in http://drupal.org/cvs?commit=377664. This is a rather serious commit and we're dealing with backwards-compatibility problems in a few places, so marking as fixed and hoping for the best.