Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Hello all,
I've been quite busy the last weeks with OpenLayers. Even if I don't commit anything, it's just because I'm stucked and trying to find a way to make things in the best way, or at least, a better way than what's existing.
Since the wonderfull patch from Augustus Kling (#1331410: Better Handling of Projections), I'm looking for a solution to ease the loading of libraries into OpenLayers.
As of today, OpenLayers's library loading is based on drupal_add_js.
We have no control, or almost no control, on the order of the loading and we cannot alter the loading order through hooks.
I had a talk with nod_ and ekes at the DrupalCamp London (which was really great btw), and even if I wasn't able to code anything there (I mostly did it in the train's journey), I had a couple of ideas to implement and test.
My main ideas and concerns are:
- Facilitate the creation of layer types and behaviors by other modules.
- Get rid of the useless code
- Get rid of
theme()function to render a map and use a renderable array. - Get rid of most of the
drupal_add_*and use the#attachedproperty of the renderable array
I have forked the 7.x-2.x branch into dclondon branch and did something.
This is what has been done so far:
- I mainly worked on
openlayers_libraries_info(),openlayers_render_map_data()andopenlayers_build_map().
Some functions like_openlayers_layers_process()and_openlayers_behaviors_render()has been altered a bit too. - The plugin definition of each layer types and behaviors has been cleaned and contains less code than before, which should be easier for people to create them.
- The
openlayers_theme()and functions that depends on it has been changed too to enable the form element. - I removed the boxes and content_types plugins, never used, they can be added back through an extra module (openlayers_blocks).
- I also reorganised the files, moved some functions into includes files to have a better visibility and also to keep the hooks only in the .module file.
- I also moved the classes from the .module in a respective file: openlayers_layer_type.class.inc and openlayers_behavior.class.inc.
- Merged #1331410: Better Handling of Projections !
Right now, displaying a map is working, but some other stuff might be broken (like geofield and some add/edit forms).
Once this and the following issue:
- #1937446: Add a 'pre-dependencies-load' hook.
- #864376: Loading of external libraries (to find a way to get Google layers working when loading a map having layers who requires external JS.), see this screenshot
are done, we'll be able to implement correctly:
- #1331410: Better Handling of Projections
- #1895530: Use the new 'openlayers' element. (which is now outdated)
- Fix bugs introduced by those changes
- Port to D8
I'm writing this post to have a feedback on what I did. Is it a good practice ? What would you do to improve it ?
Thanks!
Comments
Comment #0.0
PolUpdates
Comment #0.1
PolUpdated issue summary.
Comment #0.2
PolUpdated issue summary.
Comment #0.3
PolUpdated issue summary.
Comment #0.4
PolUpdated issue summary.
Comment #0.5
PolUpdated issue summary.
Comment #0.6
PolUpdates
Comment #1
nod_I agree with this plan.
Comment #1.0
nod_Updated issue summary.
Comment #2
augustus.kling CreditAttribution: augustus.kling commentedI tried to get the code from the dclondon branch running but it fails to execute properly. Some references are not yet right. The following comments refer to ca19092e82d26f213a306fc816e3dfa31bd59bb7.
Reproducing the one such failure is easy in the case of creating a new map:
Make sure to enable map preview, then open admin/structure/openlayers/maps/add and PHP will complain about undefined array indices.
I suspect the following causes and would be happy about explanations what you wanted happen instead.
There is no map initially, so in openlayers_maps_ui->edit_form line 13 $map gets set to NULL. Thus in line 27 the default map get loaded. I don't get why $map is not set here as well. Also, I wonder why there is no need to have a fixed map configuration as starting point given the user can delete the map called “default”.
Going down further we reach line 59 where the name property of $map is read but $map is still NULL. Unfortunately, PHP carries out but leaves $form['preview']['map']['#map'] empty.
Later in execution in openlayers_preprocess_openlayers_map the map is still not there and as a result $map is guessed to be NULL but used in various places in the same function. Couldn't this be avoided be simply providing a default map configuration in the first place? The symptom of the above is OpenLayers prints “TypeError: this.baseLayer is null” to the console instead of painting a preview map.
Now, I've tried to save the map anyway clicking “Save and edit” in the hope the default would give me a map as they seemed sensible. After the page reloads I'm still presented with the same symptom as outlined above, though.
Is the above branch assumed to be in such as state due to being work in progress or do I see a different behaviour here than you do?
Regarding facilitating the layer creation I think the base class openlayer_layer_type does too much. For example the definition of serverResolutions and resolutions should go into a subclass as it does not apply for all layer types.
In some layers, such as openlayers_layer_type_cloudmade JavaScript files get included in the render method. The static variable there is not necessary as Drupal silently ignores repeated additions of the same JavaScript file anyway. The same applies for the proposed move to #attached as it just serves as a redirect to the drupal_add_js.
I cannot find _openlayers_behaviors_process and openlayers_render_map_date in the branch. Have those been forgotten?
In my opinion this issue should be split and handled in smaller feature branches because this makes it easier for others to follow. Beside this, I assume the proposed changes to be sound. Thanks very much for your work.
Renderable arrays for those not familiar with Drupal: http://drupal.org/node/930760
Comment #2.0
augustus.kling CreditAttribution: augustus.kling commentedUpdated issue summary.
Comment #2.1
PolUpdated issue summary.
Comment #3
PolIndeed, there are still some problems. This is due to the form element and the other changes and not because I want another behavior, the module should stay exactly the same, in terms of usability, when the changes will be over.
Edit: I committed some changes, it should work now.
I though that adding
serverResolutionsandresolutionsto each layer would be a good idea, apparently not!This is because I'm still discovering all those stuff, maybe you can help to sort it out ?
This is indeed a problem, I had to keep it like this because libraries (the module) doesn't handle external url loading.
There is a issue about that: #864376: Loading of external libraries, I hope it will be sorted out, in the meantime, I have to find a another solution, which is not yet done.
Also, I had to create a patch for that module too to get it working as it should with OpenLayers, see #1937446: Add a 'pre-dependencies-load' hook..
This patch has been done because we couldn't load a variant of a library when the library is set as a dependency of another.
Indeed, my bad, these are typos, I fixed it in the first post too.
It should be:
_openlayers_behaviors_render()andopenlayers_render_map_data().I think that switching to libraries, form element and renderable array should be done in a row. That kind of changes needs to be handled in one time because they are all connected.
Thanks, your opinion is a great value for me !
Comment #4
augustus.kling CreditAttribution: augustus.kling commentedThere is no need to consider resolutions for vector layers because they can be rendered in whatever resolution is required. The serverResolutions are only needed for pre-rendered raster graphics as these are simply not available between resolutions (take this as resolutions ensure no tiles are requested in between zoom levels).
In OpenLayers serverResolutions is defined only for layers inheriting OpenLayers.Layer.XYZ. Such layers assume a fixed grid and are used with pre-rendered tiles (OSM, Google, Bing for example). See: http://dev.openlayers.org/releases/OpenLayers-2.12/doc/apidocs/files/Ope...
Note that a change is not urgent as layers that don't need the properties will simply ignore it. If we want a change here we could use separate base classes for vector and raster layers or copy OpenLayer's layer class tree. These changes can definitely wait.
Even if drupal_add_js is kept, I don't see a need to keep the static variable. drupal_add_js overrides existing settings for file and external types and therefore does not cause duplicated additions of the same file again, anyway.
Comment #4.0
augustus.kling CreditAttribution: augustus.kling commentedUpdated issue summary.
Comment #4.1
PolUpdate
Comment #4.2
PolUpdated issue summary.
Comment #4.3
PolUpdated issue summary.
Comment #5
PolThanks for your insights Augustus, I will work on the last point.
About the serverResolutions and stuff, I think I'll need a bit more time to understand and see what's best to do.
Also, I've updated the issue description.
Comment #5.0
PolUpdated issue summary.
Comment #6
Mac_Weber CreditAttribution: Mac_Weber commentedWhere is the code? It seems the dclondon branch got deleted
Comment #6.0
Mac_Weber CreditAttribution: Mac_Weber commentedUpdated issue summary.
Comment #7
PolCode is now in branch 7.x-3.x.