So, as discussed in other places, how we handle projections is crap. We sort of support 4326 for base layers, but not really. Overall we are only supporting spherical mercator (900913) with lat/lon (4326) as a display mechanism. Overall, most folks are fine with spherical mercator, but the reason to use OpenLayers is it strength to handle lots of geographical data.

For now, I am going to remove some stuff from the interface as is just confusing.

For the future, the following should happen (this is not a release blocker though):

  • Ensure that Projections can be handled in the API correctly.
  • Projections should become first class objects.
  • Support for Proj4.js
  • Make a real interface that handles projections better.

Related:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zzolo’s picture

Cleaned up map interface and hid projections a bit.
http://drupalcode.org/project/openlayers.git/commit/71206ae

mgifford’s picture

@zzolo - so is this fixed? If not what else needs to be cleaned up?

zzolo’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev

No, not fixed yet. But, now that the interface kind of hides the projection stuff, moving to 3.x.

mgifford’s picture

Just posting some links to proj4js (for me mostly):

http://trac.osgeo.org/openlayers/wiki/Documentation/Dev/proj4js
http://trac.osgeo.org/proj4js/wiki/UserGuide

Makes sense to move this to 3.x.

augustus.kling’s picture

FileSize
33.94 KB

We've recently discussed projection support, too, and do seriously consider to help. Thus I'd like to discuss some ideas we had to learn if you think they make sense for addition and if you'd be happy to accept a contribution.

First, projections should be a class of objects on their own instead of a value attached to maps and layers. To make this happen we suggest to a screen (accessible via tabs on mock) consisting of a list of projections when some can be chosen to be used in the other screens of the module. We'd further suggest to provide a hook so that other modules can provide more projections (possibly in the form of PROJ.4 like strings).

The default projections selected could be 4236 and 900913 or only 900913 to hide projections from that users that don't require to use something else than ready-made tiles.

Each layer should further allow to hold a list of projections rather than a single projection. The reason for this is that especially WMS are sometimes offered using hundreds of projections at once and you don't want to exclude those layers from map configurations where they are perfectly valid. Example of such a service: http://www.mapmatters.org/wms/594161
The projection dropdown on the settings for WMS layers could be discarded since the supported projections can be found issuing a GetCapabilities request more reliable.

For the map configuration we recommend to move the projection settings above the layer list and present those projections that have been selected on the projection screen (see above). The layer list could then be updated using a AJAX request whenever the projection changes. (Currently this does not happen and might result in invalid configurations being saved)

Please state your opinion.

ulim’s picture

Hi maintainers,

does anybody have new comments on this issue, especially on Augustus' proposal? For our work the missing projection support is a severe blocker, since it prevents the real GIS professionals from using the OL module. GIS professional means those people that have lots of own geodata and also produce data and that do this rarely in Google Mercator or WGS84 but in their local projections.

Cheers, Uli

mgifford’s picture

I'm not a maintainer, but watching this issue. The Drupal OpenLayers Module has improved a lot in the last 6 months, mostly on the 2.x branch. It's great too that there is now support for OL 2.12 in the latest dev version.

In order to get this into a 3.x release we need a plan, a patch and a lot of testing.

@Augustus presented a text version of a plan which was good. I don't know if it meets the needs of professional GIS folks, but can certainly see how it would be a step in the right direction. Would that meet your needs @ulim?

Would be great to have some sample screenshots for the UI. I do think that the UI needs to be simplified, particularly in relationship to Views. It just requires too much back/forth. That being said, would be great if it had the flexibility suggested here (which always adds complexity). It's going to take some serious thought about how to build a manageable interface.

Then we need a patch. The patch has to address this issue, but also the other enhancements that have gone in the 2.x branch. Nobody wants the 3.x to go back in functionality. There are also a bunch of related modules that are also going to need to be reworked to support the 3.x version of the code.

Once we've got that, we're going to need to test it to see that folks can upgrade, and also build in unit tests so that we can both follow best practices of Drupal coding but also have greater assurance that in adding new functionality we aren't breaking old functionality.

All of this takes developer time. That's time that mostly needs to be paid for. There are a few firms that have generously given their time to work on different aspects of OpenLayers integration, but ultimately they (and other contributors) need to be given the resources to make this a priority. It's a big job and if it needs to be done quickly then folks have to put some $$ into it.

So, other than just setting up contracts with Drupal shops to invest in the 3.x version there are also:
http://www.kickstarter.com/
https://flattr.com/

Not sure how else to make this happen, but open to ideas.

ulim’s picture

Augustus and I are working together, so his text expresses exactly what also I think would be needed.

You are absolutely right that it needs a plan (see also http://drupal.org/node/1786560) in order to get this and other new features done - and that it also needs money. However in order to start we want to develop some ideas about how to do it and collect feedback from the community if they would like it like that. Your feedback on Augustus proposal is exactly what is needed. If we get some more comments we will happily develop the concept further.

augustus.kling’s picture

Status: Active » Needs review
FileSize
36.01 KB
139.44 KB

We've decided to help with projection support and implemented it in the attached patch. Basically it provides:

  • A projection tab where new projections can be defined in addition to the default ones
  • Reloading the layer list when the projection is changed
  • Integration of Proj4js to allow for different map projections and display projections

Using WMS layers in different projections seems to work well (tested with EPSG:2056). The GUI needs to be discussed and altered as it assumes EPSG:3857 for defining the map extent which does not go well with projection support.

To try the provided projection support, define a projection in the Projections Tab unless the default ones serve your purpose.
Projection Tab
Then define a WMS layer and select the projections supported by the layer. When configuring a map, you'll be presented with all projections for which at least 1 layer is available.

Please review and comment.

Pol’s picture

Gosh what a nice patch !

Does it applies to 7.x-2.x ? Nobody is maintaining that branch and all the development are done in 7.x-2.x.

Thanks !!!!

Pol’s picture

Status: Needs review » Needs work

Looks like it works in 7.x-2.x ... almost but...

+++ b/includes/openlayers.projections.incundefined
@@ -0,0 +1,221 @@
+  $projections = array();
+  ¶

Remove tabs

+++ b/openlayers.installundefined
@@ -121,6 +121,74 @@ function openlayers_schema() {
+  // Projection table (ctools extras)

Don't forget to provide an update function, this will be mandatory if you want this in 7.x-2.x.

+++ b/openlayers.installundefined
@@ -121,6 +121,74 @@ function openlayers_schema() {
+      'identifier' => array(
+        'type' => 'text',
+        'not null' => TRUE,
+        'description' => 'Opaque identifier. Guaranteed to be unique but does not have any other meaning.'

This should be varchar and add a length, just like in layers and maps.

Once this is done, I have an error when I try to go on the map tab:

Notice: Undefined offset: 1 in openlayers_layer_type->__construct() (line 1168 of /home/pol/git/openlayers/openlayers.module).
Exception: Projection 0: requested but not supported in openlayers_get_projection_by_identifier() (line 950 of /home/pol/git/openlayers/openlayers.module).

Once again, congrats for this huge work, it was in my todo list and it feels good to have a bit of help !!!

Thanks !

Pol’s picture

I did a better review of the code today.

I still have some questions:

  1. Shouldn't we bundle Proj4JS as a library instead of copying it in the module ?
  2. Does your patch handle non spherical mercator projections too ? On the map form, in the "Layers & Styles" tab, can we remove the "Warning: Projections are not well supported..." ?
  3. The default map is now using 'EPSG:3857' instead of 'EPSG:900913'. Now, when I load the default map, I have some broken images, See the screenshot.

More stuff will follow probably :-)

augustus.kling’s picture

Thanks a lot for your comments. I've started to improve the patch but it's not yet ready for another review. Hopefully I do find the time to complete the changes tomorrow.

I will get back to you shortly to address some questions regarding Drupal-speak to make sure I understand you well.
Regarding your questions:
1. I don't know the implications or either option. We should just make sure that Proj4js is available when the OpenLayers module is installed because otherwise transformations will go wrong for all projection but those supported natively by OpenLayers.
2. Non-spherical Mercator projections are supported as well. I can load WMS layers in EPSG:2056 for example. However I suggest to keep the warning for now until the GUI (e.g. restricted extents) is ready for various projections, too (needs to follow in another patch).
3. This is due to the lacking migration of the map configurations. Please recheck after I submitted the corrections to the initial patch.

Please wait for the next revision of the patch before conducting further tests as a few more things will be handled. Thanks a lot for your responsiveness.

Pol’s picture

Excellent, can't wait to see that :-)

Thanks !!!

Pol’s picture

Just thinking to something...

When exporting a layer, the projection should only contain the identifier, not the full object.

See screenshot

What do you think ?

augustus.kling’s picture

Status: Needs work » Needs review
FileSize
143.34 KB

It's time for the next iteration and some questions.

Things changed:

  • Migration of existing configurations added (Did you mean this by “update function”?)
  • Removed Windows-style line breaks (as this breaks Drupal's MySQL tests for whatever reason)
  • Keeping EPSG:900913 more often, just in case some server does not yet know about EPSG:3875
  • Layers' projections are exported as identifiers

My questions / Open things:

  • I can't find the tabs you've referred to in comment 11. Can you recheck?
  • What's the reasoning in favour of VARCHAR with a fixed length instead TEXT as column type? Does this MySQL have a problem with the latter?
  • How to we deliver Prj4js? It's simply a file within the OpenLayers module, currently.
  • For a follow up patch: What GUI do we want to have for the “Center & Bounds” section of the map configuration? The current GUI allows to create extents and positions that violate the map projection. I'm simply dropping the user's entries if they violate the projection's constraints for now.

I'm looking forward to any comments.

Status: Needs review » Needs work

The last submitted patch, openlayers-projections-1331410-16.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review

Testing right now !

Pol’s picture

Status: Needs review » Needs work

It seems to be better but...

  • Migration of existing configurations added (Did you mean this by “update function”?) Ok.
  • Removed Windows-style line breaks (as this breaks Drupal's MySQL tests for whatever reason) Ok.
  • Keeping EPSG:900913 more often, just in case some server does not yet know about EPSG:3875: I still have the same problem.
  • Layers' projections are exported as identifiers Ok.

I can't find the tabs you've referred to in comment 11. Can you recheck?

There are still tabs and or trailing spaces in the patch, we have to clean them, but it's not the most important.

What's the reasoning in favour of VARCHAR with a fixed length instead TEXT as column type? Does this MySQL have a problem with the latter?

Just to be consistent with layers and maps.
Also, if you set it as TEXT, when installing you'll get an error because it needs a length... I know it's weird.

How to we deliver Prj4js? It's simply a file within the OpenLayers module, currently.

I'm thinking about doing a small module who provide Proj4JS as a library (a small module, just like JpGraph). I'll do it tonight probably.
We'll probably need to do something just in case if that module is not there. For example not be able to use or display layers with Projection different from EPSG:4326 and EPSG:900913.
If not, we can add proj4js(the module when it will be done) as a dependency for OpenLayers.

For a follow up patch: What GUI do we want to have for the “Center & Bounds” section of the map configuration? The current GUI allows to create extents and positions that violate the map projection. I'm simply dropping the user's entries if they violate the projection's constraints for now.

I don't have a lot of knowledge with projection, so I'm absolutely confident with what you will propose.
What is the best solution ?

Thanks again !

augustus.kling’s picture

Again, thanks a lot for your comments.

grep still can't find any tabs. I did not check for trailing spaces but will try to get a Drupal code style checker running next week – hopefully, it will get rid of such things.

Also, if you set it as TEXT, when installing you'll get an error because it needs a length

I'm using TEXT without a length here and it works just fine. PostgreSQL's manual also confirms that it's of unlimited length. Which database are you running?
Beside that, consistency is a good point.

Regarding Proj4js, I suggest waiting for the availability of the mini-module then and add it as dependency. Each “if” less, the better.

A proposal regarding the GUI changes will follow (latest next week).

Thanks again and have a good week-end.

Pol’s picture

I made the Proj4JS module, I just did this in a hurry and I've set you maintainer.

Example of use:

/**
 * Load projection transformations in case OpenLayers does not support projections in use natively
 * @param openlayers_projection $projection
 */
function openlayers_add_js_projection_definition(openlayers_projection $projection){
  $openLayerNativelySupported = array('EPSG:4326', 'EPSG:900913');
  // Only load Proj4js if projection not supported by OpenLayers anyway
  if(!in_array($projection->identifier, $openLayerNativelySupported)){
    proj4js_load_definition($projection->identifier, $projection->getDefinition());
  }
}

You can also use proj4js_load_definitions(), who take an array as argument.

augustus.kling’s picture

Pol, thanks a lot for packing the proj4js module. I'll integrate it in the next patch iteration (sometime in the early upcoming week).

Pol’s picture

Version: 7.x-3.x-dev » 7.x-2.x-dev

Hello @augustus,

Just FYI: I made some changes in OpenLayers, it's now using Libraries to load the library. You might encounter some problems when rerolling your patch.

Have a nice day!

augustus.kling’s picture

Status: Needs work » Needs review
FileSize
92.5 KB

Please find attached a patch that can be applied to 7.x-2.x after your changes to integrate the libraries. It does also use the proj4js module instead of providing Proj4js itself and uses VARCHAR as column type.

I have however been unable to reproduce the projection objects in the layer export. Whenever I tried I got projection identifiers as strings. My approach to reproduce was installing the openlayers module before the projection patch, then creating layers. Afterwards I applied the patch, used Drupals update.php to migrate the database and invoked the layer export.
Can you elaborate how to reproduce the objects in the layer export?

Pol’s picture

I made a boo boo sorry.

Pol’s picture

Status: Needs review » Needs work

This is very good ! There are still some trailing spaces but it's not a big deal, your patch add a great value to the module.

I just tried it, it works pretty good.

However, I'm still having the problem see in the screenshot for the default map. Do you have it too ?

About Proj4JS Module, do you think we should do something more or it's ok ?

I'll commit the patch as soon as we have something fixed for the default map (screenshot).

Thanks a lot !

augustus.kling’s picture

FileSize
55.14 KB

Can I just blindly discard any trailing spaces in the openlayers module or are there some that need to be kept?

This issue from your screenshot is what I could not reproduce. Do you also try the patch in the same way as outlined in comment 24? I'd be happy for any hints how to reproduce what you are seeing.
Your screenshot shows the export of a single layer, not the default map, by the way.

The export looks like shown in the following screenshot whenever I try:
Layer export

Regarding the proj4js module: I think it already does enough and was working just fine here. We could maybe do even less because providing solely proj4js-compressed.js would be sufficient. I am however not familiar with providing libraries in Drupal so I can't really judge if doing less would be better.

I'm looking forward to integrate a new build of the openlayers module into Cartaro but first I need to get the layer export fixed.

Pol’s picture

I'm sorry Augustus, I copy paste the wrong link, here is a screenshot of the problem.
The issue described in my previous post, with the wrong screenshot is solved, so, no worry.

Regarding the trailing spaces, don't worry, it can be done separately in another issue.

augustus.kling’s picture

Status: Needs work » Needs review
FileSize
94.83 KB

It seems the missing tiles are caused by disabling the dateline wrapping. As soon as the wrapDateLine property of a layer is set to true, the problem goes away.

Note that the projection for proj4js is loaded manually again in openlayers_add_js_projection_definition because the dependency handling of the libraries module did not work properly. We need loading of Proj4js only if OpenLayers does not support the projections natively, and projection definitions emitted in between the Proj4js and OpenLayers libraries. I could not get the libraries module to fulfil this requirements. Do you know if it can handle it at all? Beside that, I'm not aware of any more bug in the projection handling.

Pol’s picture

Status: Needs review » Needs work

Hello Augustus,

I don't get you with the Proj4JS stuff, could you explain a bit more ?
Which dependency handling in libraries ?

AFAIK, we only load Proj4JS when we need it, through the pro4js_load_definition() function, right ?
And also, the Proj4JS module load the JS who doesn't need any dependency.

Thanks !

augustus.kling’s picture

Thanks for your comment.

The dependency stuff is about having a guaranteed execution order. The following order is crucial where equal numbers denote that the order does not matter:
1.) Proj4js library
2.) Projection definitions (this is where I had the inline script and your module uses the $settings array)
2.) OpenLayers library
3.) Map initialization

In the simplest case (when only EPSG:900913 and EPSG:4326 are in use) there is no need to load Proj4js nor projection definitions as OpenLayers can handle those projections already.

I could not guarantee that the projection definitions are in place before the map is created by only using the library module. The error cause is that the projection definitions and the map initialization are coming via Drupals behaviors which don't guarantee execution order.
If the map initialization comes first, OpenLayers tries to build its map with unknown projection definitions. In such a case you'll see that Proj4js, if present, tries to include the definitions by loading an external script file for each projection. Even if it succeeds these are unnecessary requests and more importantly not the user's projections definitions (though likely equal). If it does not succeed we have unnecessary requests to get inexistent files and the OpenLayers gets coordinate transformations wrong. It might be that wrong calculations are not obvious and only result in wrong display of mouse coordinates for example.

The manual generation of the JavaScript in openlayers_add_js_projection_definition works around any known problems by guaranteeing the execution order. It does also load Proj4js only when required.
One could say that openlayers-projections-1331410-29.patch contains a workaround for the outlined problem and will thus work in practice.

I'd prefer to guarantee the order with the library module alone and omitting the manual JavaScript generation. However I did not yet find an API to do so.

Pol’s picture

I see the problem now.

I tried to do:

/**
 * Implements hook_libraries_info().
 */
function openlayers_libraries_info() {
  $libraries['openlayers'] = array(
    'name' => 'OpenLayers',
    'vendor url' => 'http://openlayers.org/',
    'download url' => 'http://openlayers.org/download/OpenLayers-2.12.tar.gz',
    'version arguments' => array(
      'file' => 'lib/OpenLayers.js',
      'pattern' => '/OpenLayers.VERSION_NUMBER="Release (.*?)"/',
      'lines' => 417,
    ),
    // Adding proj4js as a dependency.
    'dependencies' => array(
      'proj4js',
    ),
    'files' => array(
      'js' => array('OpenLayers.js'),
    ),

But it doesn't work. I'll try to find a cleaner solution these days.

dianacastillo’s picture

I have time to do some testing if you guys need help, what do you want me to do?

augustus.kling’s picture

Thanks for your offer Diana. I think the parts that would most likely benefit from testing is if layers and maps are properly modified during updates.

To test install the openlayers module from the 7.x-2.x branch and create some layers. Now, backup your database, apply the patch and invoke Drupal's update script. Please report any issues that arise whilst the update script is running. Afterwards verify that your layers and maps are still working.

It would help a lot to reproduce if you could provide stack traces from the update script or from within the openlayers module after applying the patch. In case the migration of the data goes wrong it would also help if you could provide a dump of the openlayers_layers, openlayers_maps and openlayers_projections tables so that the issue can be reproduced easily.

Thanks a lot for helping us out.

Pol’s picture

Hello Augustus,

I'm making some examples of the new functionnalities on a demo site and, when I apply the patch, Bing layers types are no more working.
Also, it's impossible to display this file using KML layer type (EPSG:4326). It looks like it's a problem with projection. (See it working good here)

I will try to see where it comes from.

augustus.kling’s picture

The revised patch fixes the problem with the reprojection of the KML file. Therefore your KML can now be overlayed with any base map (choose EPSG:4326 in the KML's layer configuration).
Additionally the missing dependency for the upload of the KML file (file module) has been added.

I did not yet find the time to look into the Bing issue.

Pol’s picture

Perfect it works indeed :-)

Some remarks:

  • For the next iteration of the patch, we'll need to include this: http://drupalcode.org/project/openlayers.git/commit/0004807, I committed it 1 hour ago.
  • ctools_include('export'); missing in the function openlayers_get_projection_by_identifier().
    About that, shouldn't we use the same structure as in openlayers_map_load() ?

    Quick example (not tested):

    /**
     * @param String $identifier Opaque identifier
     * @return openlayers_projection
     */
    function openlayers_get_projection_by_identifier($identifier) {
      ctools_include('export');
    
      $record = ctools_export_load_object('openlayers_projections', 'names', array($name));
    
      if ($record===NULL) {
        throw new Exception("Projection $identifier requested but not supported");
      }
      return openlayers_projection_from_record($record);
    }
    

    I know it's a small thing, but it's just that the lesser external functions we add, the easier it will be for other people to understand the code.
    In this case, we remove the reference to the ctools_export_crud_load() function.

  • And lastly, we should find a way to remove the duplicates here:
    <script type="text/javascript">
    <!--//--><![CDATA[//><!--
    Proj4js.defs["EPSG:3857"]="+proj=merc +a=6378137 +b=6378137 +lat_ts=0.0 +lon_0=0.0 +x_0=0.0 +y_0=0 +k=1.0 +units=m +nadgrids=@null +wktext  +no_defs";
    //--><!]]>
    </script>
    <script type="text/javascript">
    <!--//--><![CDATA[//><!--
    Proj4js.defs["EPSG:3857"]="+proj=merc +a=6378137 +b=6378137 +lat_ts=0.0 +lon_0=0.0 +x_0=0.0 +y_0=0 +k=1.0 +units=m +nadgrids=@null +wktext  +no_defs";
    //--><!]]>
    </script>
    <script type="text/javascript">
    <!--//--><![CDATA[//><!--
    Proj4js.defs["EPSG:3857"]="+proj=merc +a=6378137 +b=6378137 +lat_ts=0.0 +lon_0=0.0 +x_0=0.0 +y_0=0 +k=1.0 +units=m +nadgrids=@null +wktext  +no_defs";
    //--><!]]>
    </script>
    

    I'm thinking about a static variable, but I know that it's not a good pratice, any other idea ?

  • Bing map stuff are fixed, the API key was missing, totally unrelated to the patch, sorry about that.

Thanks !

augustus.kling’s picture

Thanks for that proposals. I had a look into the Bing stuff, too, but found that it was not possible to add a Bing key due to forgotten imports – these have now been added along with some other imports that have been missing.
It was not obvious that the Bing keys were missing because OpenLayers does not propagate Bing's error message. OpenLayers.Layer.Bing.processMetadata should throw a meaningful error in that case but that's clearly out of scope here.

I've replaced openlayers_get_projection_by_identifier with an implementation that is similar to your proposal.

I did not yet approach the duplicated projection definitions because they don't cause any problems (except for the few additional bytes transferred). Given Drupal's concepts and PHP's limits a static variable seems like the best way to get rid of the duplicates because it keeps the state in a local place and avoids relying on global variables.

Pol’s picture

That's very good !!!

I have found a way to get rid of the inline JS, using the libraries.

We just need to add a dependency to proj4js when loading the OpenLayers library and bingo !

/**
 * Implements hook_libraries_info().
 */
function openlayers_libraries_info() {
  $libraries['openlayers'] = array(
    'name' => 'OpenLayers',
    'vendor url' => 'http://openlayers.org/',
    'download url' => 'http://openlayers.org/download/OpenLayers-2.12.tar.gz',
    'version arguments' => array(
      'file' => 'lib/OpenLayers.js',
      'pattern' => '/OpenLayers.VERSION_NUMBER="Release (.*?)"/',
      'lines' => 417,
    ),
    'dependencies' => array(
      'proj4js'      
    ),

What do you think if proj4js is a mandatory dependency of OpenLayers ?

augustus.kling’s picture

I assumed that specifying a dependency results in loading the dependency before the library. That would mean that Proj4js is always included when OpenLayers is included. We should however only include Proj4js when actually needed (when projections are in use that can't be handled by OpenLayers alone).

Please correct me in case my assumption is wrong. If the assumption is wrong I'm happy with specifying the dependency.

Pol’s picture

You're perfectly right, it may add some JS loaded when it's not needed, which is a bad practice.

What we could do, is to detect when we need proj4js through a function, and update openlayers_include() in such a way:

function openlayers_include() {
  if ('internal' == variable_get('openlayers_source_type', 'external')) {
    $variant = variable_get('openlayers_source_internal_variant', NULL);
    if ($variant == 'original') $variant = NULL;

    if ( detect_if_we_need_proj4js() ) {
      libraries_load('proj4js');
    }

    libraries_load('openlayers', $variant);
  }

Another solution would be to create a new variant of the OpenLayers library: 'openlayers proj4js' and this variant will add 'proj4js' as a dependency when needed.

/**
 * Implements hook_libraries_info().
 */
function openlayers_libraries_info() {
  $libraries['openlayers'] = array(
    'name' => 'OpenLayers',
    'vendor url' => 'http://openlayers.org/',
    'download url' => 'http://openlayers.org/download/OpenLayers-2.12.tar.gz',
    'version arguments' => array(
      'file' => 'lib/OpenLayers.js',
      'pattern' => '/OpenLayers.VERSION_NUMBER="Release (.*?)"/',
      'lines' => 417,
    ),
    'files' => array(
      'js' => array('OpenLayers.js'),
    ),
    'variants' => array(
      'openlayers proj4js' => array(
        'dependencies' => array(
          'proj4js'
        ),
        'files' => array(
          'js' => array(
            'OpenLayers.js'
          ),
        ),
      ),

I prefer the first solution, what do you think ?

Pol’s picture

Hello Augustus,

Here's a quick overview of the things to do before including this patch: #1936036: Improve the library loading and use renderable array..
I'm looking for reviewers and help.
Once it will be done, we'll have a clean loading of library and we'll be able to add proj4js as it should be.

Thanks!

augustus.kling’s picture

Given the broad scope of the proposals in issue #1936036 I'm in favour of merging the patch before approaching all the changes. I fear that implementing the proposals before applying the patch is going to cause a lot of work with trying to put all the pieces of projection handling at the right place afterwards.

To me it seems a lot easier to apply the working patch before adjusting big portions of the code. Doing the changes afterwards will take the projection stuff to the correct places in the process and avoids having a pending patch around that get less and less applicable with the changes.
However loading Proj4js with the library system is to be provided after your proposals are in, obviously.

I will comment in more detail on your proposals in #1936036 after I had a closer look on the diff of 7.x-2.x to dclondon.

augustus.kling’s picture

Given that library loading in the patch does work (it's just not using the libraries module yet) I will now go on integrating the projections patch. Then library loading will be modified so that the libraries module will get used. The latter is probably done as outlined in #1937446.

Pol’s picture

Assigned: Unassigned » augustus.kling

Perfect :-)

augustus.kling’s picture

The patch is now in. Please report any issues and things that require clarification so that proper and understandable documentation can be written.

augustus.kling’s picture

A documentation draft is now available at http://drupal.org/node/1939470.

Anonymous’s picture

Hello,

with the last dev, I have the following error trying to create a map:

Exception : Projection 900913 requested but not supported dans openlayers_get_projection_by_identifier()

Anonymous’s picture

It seems that the object is named 900913 in place of EPSG:900913.

augustus.kling’s picture

Status: Needs work » Active

Can you please verify that your download contains a function openlayers_update_7208 in openlayers.install? Did you migrate existing data by invoking Drupal's update.php script?

Anonymous’s picture

Status: Active » Needs work

I have that function.

I didn't any upgrade. I used drush.
drush dis openlayers
drush pm-uninstall openlayers
drush dl openlayers-7.x-2.x-dev
drush en olfp openlayers_proximity openlayers_ui openlayers_views

Maybe, there is a conflict with olfp and proximity ?

Anonymous’s picture

I just did a test without olfp and proximity. Nothing changes.

Anonymous’s picture

So, I think I will go back to the last stable version for now on.

Pol’s picture

I didn't any upgrade. I used drush.
drush dis openlayers
drush pm-uninstall openlayers
drush dl openlayers-7.x-2.x-dev
drush en olfp openlayers_proximity openlayers_ui openlayers_views

This is why it doesn't works.

There are new functionnalities in the -dev version which requires you to run drush updb.
Also, you have to install Libraries (2.x) and Proj4JS.

After these steps are made, you should be good to go.

Anonymous’s picture

I did the db upgrade and Proj4JS is installed as Libraries but the problem is still there.

Anonymous’s picture

Sometimes the error changes and I have this one instead:

Notice : Undefined index: dans openlayers_get_projection_by_identifier() (ligne 969
Notice : Trying to get property of non-object dans openlayers_projection_from_record() (ligne 936

Pol’s picture

Then the problem comes from a module who provides a layer who doesn't use the new syntax, do you have geofield installed ?

Anonymous’s picture

Yes, I have. And views_geojson.

Pol’s picture

Try to disable them for now and see if you still have the problem.

Anonymous’s picture

It seems working without geofield. But, I need it, what can I do ?

Anonymous’s picture

I have the 7.x-2.0-alpha2 version for geofield. Do I have to upgrade or go back to the 7.x-1 branch ?

Pol’s picture

In the layer type class and/or map definitions, replace '900913' by 'EPSG:900913', it should work afterwards.

Pol’s picture

In the file geofield.openlayers.inc,

replace all occurence of :

    'projection' => '900913',
    'displayProjection' => '4326',

with

    'projection' => 'EPSG:900913',
    'displayProjection' => 'EPSG:4326',
Anonymous’s picture

I'm sorry I didn't understand very well the last post. Where is that layer type class ?

Anonymous’s picture

It works now.

Thanks a lot.

Anonymous’s picture

I will address that issue to geofield for them to modify in the dev.

Anonymous’s picture

Hello,

sorry but I have a little problem with that new configuration.

When I create a content with a geofield geocoding from an addressfield, I want to display that field with an openlayers map but the maps created with the geofield placeholder overlay are not displayed.

I have a white space instead.

Pol’s picture

Hello Augustus,

I think we should remove the description from the checkboxes, what do you think ? See the screenshot.

Thanks!

augustus.kling’s picture

You're right. They should be removed because they show the same information as the “Overlay Layers” column.

Do you want to do it or shall I?

Pol’s picture

I let you take care of that.

If you want, you can also commit the changes on the dclondon branch too !

Thanks!

augustus.kling’s picture

The duplicated labels have been removed in a6bd2cd9d994ede2537798f16df5e7f81b69ebdf.

Rob C’s picture

@Pol

About #63 - i did this, and it indeed works like expected. Created a patch for geofield, see http://drupal.org/node/1942826#comment-7183662

Seems ok, include the patch from #comment-7183662 when testing with geofield.

Anonymous’s picture

I have the following errors with the last dev:

Notice : Undefined property: stdClass::$name dans openlayers_layer_sanity_check() (ligne 397 dans sites/all/modules/openlayers/openlayers.module).
Notice : Undefined variable: map dans openlayers_layer_sanity_check() (ligne 399 dans sites/all/modules/openlayers/openlayers.module).
Notice : Undefined property: stdClass::$name dans openlayers_layer_sanity_check() (ligne 397 dans sites/all/modules/openlayers/openlayers.module).
Notice : Undefined variable: map dans openlayers_layer_sanity_check() (ligne 399 dans sites/all/modules/openlayers/openlayers.module).

I can't see any map layer anymore.

Anonymous’s picture

Ok nevermind for the last errors. they are gone without explanations.

But I still don't have the geofield map widget displayed.

Can you help me ?

Pol’s picture

Hi Augustus,

I'm upgrading a site (OpenLayers-7.x-2.0-dev-100) to dev version from git and got this error during the update:

Update #7207
Failed: PDOException : SQLSTATE[HY000]: General error: 1364 Field 'description' doesn't have a default value: INSERT INTO {openlayers_layers} (data) VALUES (:db_insert_placeholder_0); Array ( [:db_insert_placeholder_0] => a:1:{s:10:"projection";a:0:{}} ) dans drupal_write_record() (ligne 7106 dans /srv/data/web/vhosts/www.atomescrochus.pl/htdocs/includes/common.inc).
augustus.kling’s picture

I currently do not see how this error message is related to the projection related changes. The definition of table openlayers_layers does not have a default value for its description column and therefore requires every layer to have an explicit description.

It seems like one of your layers is not having a description and that this lack does only get apparent because the update introduced with the projection causes the layer to be saved in the database for the first time. I suspect ctools was retrieving a layer from a PHP file and is now required to store the layer in the database because update 7207 changes the srs or projection properties of the layer. Storing the layer fails because its definition is incomplete according to the table definition and not because of anything projection related. You simply did never notice the layers incompleteness because ctools has never checked its definition was valid – your database is stricter and refuses to store.

In order to find the offending layer please provide a stack trace and ensure it contains the state of $layer in openlayers.install line 523 when the error strikes. If you can't, dump the states of $layer and watch for a layer that lacks a description property or has it set to NULL.

Fixing the issue would then mean setting a description property for the layer or change the table definition so that the description column carries the empty string as default value.

Anonymous’s picture

Each time I upgrade I have the errors I posted before concerning sanity_check.

Can you please post something to heal that ?

Thanks.

Pol’s picture

Hello Augustus,

Thanks for the explanation.
I was upgrading the OpenLayers module at a client and there were a problem during the upgrade.
I tried to fix the problem by setting a default description, but another problem came.

I ended up by emptying the openlayers_layers table and create something new, from scratch, and all went perfectly well.

I have spotted another thing in the code, I need your input.

in openlayers.module, function openlayers_render_map_data():

  //...
  // Return themed map if no errors found
  if (empty($map['errors'])) {
    // In case the layer offers the same projection as the map, use this and do not provide
    // projection definition to client. Otherwise rely on the client to reproject on the fly.
    foreach ($map['layers'] as $layer_name => $layer) {
      if(in_array($map['projection'], $map['layers'][$layer_name]['projection'])){
        $map['layers'][$layer_name]['projection'] = $map['projection'];
      } else {
        // Client is able to reproject any possible projection because their definitions need to be
        // known to be able to set up a layer with a certain projection. Thus choice does not matter.

        // Using this:
        $layerProjectionIdentifier = array_shift($map['layers'][$layer_name]['projection']);

        // instead of:
        $layerProjectionIdentifier = $map['layers'][$layer_name]['projection'][0];

        $map['layers'][$layer_name]['projection'] = $layerProjectionIdentifier;
        // Provide client with projection definition so that it can reproject
        openlayers_add_js_projection_definition(
          openlayers_get_projection_by_identifier($layerProjectionIdentifier)
        );
      }
    }
 //...

I used the array_shift() function to get the projection instead of using the index '0'.
Sometimes, the index is the projection identifier itself, and so, using [0] is causing troubles.

I will try to do it again at home and explain how to reproduce the bug hereunder asap, in the meantime, you can already tell me what you think.

augustus.kling’s picture

Let me know how to reproduce a non-numeric key in the projection array. This should not happen.

The plan is to give projections as list such as array('EPSG:3857', 'EPSG:900913') as this is the case with the supplied layers. I don't see a use for using string keys and so this should not be done in order to keep the code as simple as possible.

I suggest ensuring selected projections are always saved as numerically keyed array instead of changing openlayers_render_map_data. Do not use array_shift because it modifies the passed array.

Pol’s picture

Hi Augustus,

I agree.

How to reproduce it, I just found an easy way to do it:

  1. Edit the layer "Example GeoJSON, "Picture This"
  2. Do not change anything, just click on "Save"
  3. Bummer.
augustus.kling’s picture

It turned out Drupal's form API returns the selected values as string keyed array instead of a numerically keyed array. The following change fixes the problem but it seems hacky. Are you aware of a better way to get a numerically keyed array from select boxes?

diff --git a/openlayers.module b/openlayers.module
index 03cf483..2828839 100644
--- a/openlayers.module
+++ b/openlayers.module
@@ -1267,7 +1267,7 @@ class openlayers_layer_type {
    * @param array $default
    */
   function options_form_submit($form, &$form_state) {
-
+    $form_state['values']['data']['projection'] = array_keys($form_state['values']['data']['projection']);
   }
 
   /**
Pol’s picture

I think that's the only way to do.

Be aware that some layer types, like the KML layer type is using it's own options_form_submit method.
Maybe we should add a parent::options_form_submit(); before ?

What do you think ?

augustus.kling’s picture

The KML layer does now call options_form_submit of its base layer. This should be enough to cover all layers as no other layer has its own options_form_submit implementation.

Whilst applying the changes I noticed that openlayers_layers_ui's edit_form_submit checks for the existence of a options_form_submit method before calling it. I would say the check is not necessary as the method is defined in openlayers_layer_type and therefore available for all layers. Is there something I'm overlooking?

Pol’s picture

Good, thanks Augustus!

Are you talking about this bit of code in modules/openlayers_ui/plugins/export_ui/openlayers_layers_ui.class.php ?


 /**
   * Prepare the tag values before they are added to the database.
   */
  function edit_form_submit(&$form, &$form_state) {
    $layer = openlayers_layer_type_load($form_state['values']['data']['layer_type']);
    if (method_exists($layer, 'options_form_submit')) {
      $layer->options_form_submit($form, $form_state);
    }

    parent::edit_form_submit($form, $form_state);
  }

Indeed this can be rewritten correctly now.

augustus.kling’s picture

Thanks, I've removed the obsolete check in 6df04b79c1dcfe7922c11d9c418a3953ffcea120.

Maksym Kozub’s picture

It seems like one of your layers is not having a description and that this lack does only get apparent because the update introduced with the projection causes the layer to be saved in the database for the first time. I suspect ctools was retrieving a layer from a PHP file and is now required to store the layer in the database because update 7207 changes the srs or projection properties of the layer. Storing the layer fails because its definition is incomplete according to the table definition and not because of anything projection related. You simply did never notice the layers incompleteness because ctools has never checked its definition was valid – your database is stricter and refuses to store.

I got the same error upon upgrading from beta3 to beta4 with Proj4JS. I then downgraded back to beta3, disabled Proj4JS etc., but the error is still there. Is emptying the layers table my only solution now? I tried running update.php after downgrading to beta3, and it says I have no pending updates. Does it mean that update 7207 is somehow irreversible?
Sorry if my questions sound stupid. I have been using OpenLayers with Geofield for many months, I also tried it with Geofile, and everything worked successfully until I tried beta4 with this projections stuff.

augustus.kling’s picture

The updates are irreversible – this is why Drupal asks you to backup your data before applying the updates. Note that this behaviour is not special for 7207 but applies for all updates carried out by Drupal's update.php. Doing a proper downgrade means reverting your code and restoring your database from the backup.

I've have never seen the described error with the missing description myself and nobody posted a sample to reproduce nor the modules they use when the problem becomes apparent. Thus I've checked the layers that are delivered with the openlayers modules and those are fine. This makes it likely that a third-party module provides a layer definition that is simply incomplete.

Note that the geofield module was published before the beta4 of the openlayers module. This means their extensions to the openlayers module are perhaps untested (which means the combination of the 2 modules might not work). In case problems occur save the failing state for a bug report and revert to a working backup.

Options to help us help you:

  • Post a list of your enabled modules (with versions) and possibly your openlayers_layers table.
  • Provide a stack trace of the failing update that includes method/function parameters.
  • In case you're using geofield, edit geofield.openlayers.inc so that its maps and layers contain a projection with an authority code and have a non-empty description (see http://drupal.org/node/1944074). Then try if update.php can apply its updates successfully.

Sorry about that, but we can only help if we have precise information or sample data at hands.

Maksym Kozub’s picture

Upgraded OpenLayers again to beta4; update.php went fine, with no pending updates. Edited those projection codes in geofield.openlayers.inc to include "EPSG:", as described here and in the other thread you mentioned. Map preset previewed fine at http://personal.kozub.in.ua/admin/structure/openlayers/maps/list/MyMapNa..., but the actual nodes still show white boxes instead of maps.
Modules installed and enabled:

drupal 7.21
advanced_help 7.x-1.0
botcha 7.x-3.2
calendar 7.x-3.4
context 7.x-3.0-beta6
ctools 7.x-1.2
date 7.x-2.6
Date iCal 7.x-1.1
Devel 7.x-1.3
Disable breadcrumbs 7.x-1.3
Domain Access 7.x-3.9
domain_blocks 7.x-3.0-alpha1
domain_views 7.x-1.5
domaincontext 7.x-1.0-alpha1
entity 7.x-1.0
entity_translation 7.x-1.0-beta2
exclude_node_title 7.x-1.6
fb 7.x-3.3-beta6
geocoder 7.x-1.2
geofield 7.x-1.1
geofile 7.x-1.x-dev (2012-Oct-24)
geophp 7.x-1.7
i18n 7.x-1.8
lang_dropdown 7.x-1.5
libraries 7.x-2.1
link 7.x-1.1
ljxp 7.x-1.0
metatag 7.x-1.0-beta5
moopapi 7.x-1.2
node_clone 7.x-1.0-rc1
oauth 7.x-3.1
openlayers 7.x-2.0-beta4
panels 7.x-3.3
proj4js 7.x-1.2
redirect 7.x-1.0-rc1
title 7.x-1.0-alpha7
token 7.x-1.5
twitter 7.x-5.6
user_relationships 7.x-1.0-alpha5
variable 7.x-2.2
views 7.x-3.6
vkxp 7.x-1.0-rc4

The site is really simple, and, as I said before, it was working fine up to OL beta3, so I believe the whole problem is somewhere between Geofield/OL/Proj4JS.
Any help much appreciated. I can provide admin credentials if needed. Getting crazy seeing these white boxes where I used to have our GPX data nicely shown on top of a Google map...

Pol’s picture

I'm working on it, I'm also having the problem now.

RdeBoer’s picture

Hi Pol, Augustus et al,

If all that is being changed as far as the API to other modules is concerned, is a prefix, then surely the new code could insert a default prefix if the projection name received doesn't start with a digit or doesn't have a colon in it?

That would save users and maintainers alike a lot of head-aches...

What am I missing?

Rik
maintainer of IP Geolocation Views and Maps, and getting lots of people whinging ....

Pol’s picture

Hello Rick,

We know about the problem, we know that it would cause such things because we changed the API, for a greater good.

Adding a condition in our code will prevent module developers to update their code and that's not what we want (Augustus and I also explained it here).

The second thing that we found yesterday is about the maxExtent on maps and we are looking for a solution already.

Expect a beta5 soon, which will fix those problems hopefully.

Thanks.

Pol’s picture

Status: Needs work » Fixed

I'll close it for now, assuming that it's closed.

Status: Fixed » Closed (fixed)

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