Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
Comment | File | Size | Author |
---|---|---|---|
#44 | openlayers-projections-1331410-44.patch | 95.34 KB | augustus.kling |
#38 | openlayers-projections-1331410-38.patch | 96.01 KB | augustus.kling |
#36 | openlayers-projections-1331410-36.patch | 96.64 KB | augustus.kling |
#34 | openlayers-projections-1331410-34.patch | 94.89 KB | augustus.kling |
#24 | openlayers-projections-1331410-24.patch | 92.5 KB | augustus.kling |
Comments
Comment #1
zzolo CreditAttribution: zzolo commentedCleaned up map interface and hid projections a bit.
http://drupalcode.org/project/openlayers.git/commit/71206ae
Comment #2
mgifford@zzolo - so is this fixed? If not what else needs to be cleaned up?
Comment #3
zzolo CreditAttribution: zzolo commentedNo, not fixed yet. But, now that the interface kind of hides the projection stuff, moving to 3.x.
Comment #4
mgiffordJust 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.
Comment #5
augustus.kling CreditAttribution: augustus.kling commentedWe'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.
Comment #6
ulim CreditAttribution: ulim commentedHi 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
Comment #7
mgiffordI'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.
Comment #8
ulim CreditAttribution: ulim commentedAugustus 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.
Comment #9
augustus.kling CreditAttribution: augustus.kling commentedWe've decided to help with projection support and implemented it in the attached patch. Basically it provides:
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.
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.
Comment #10
PolGosh 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 !!!!
Comment #11
PolLooks like it works in 7.x-2.x ... almost but...
Remove tabs
Don't forget to provide an update function, this will be mandatory if you want this in 7.x-2.x.
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:
Once again, congrats for this huge work, it was in my todo list and it feels good to have a bit of help !!!
Thanks !
Comment #12
PolI did a better review of the code today.
I still have some questions:
More stuff will follow probably :-)
Comment #13
augustus.kling CreditAttribution: augustus.kling commentedThanks 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.
Comment #14
PolExcellent, can't wait to see that :-)
Thanks !!!
Comment #15
PolJust thinking to something...
When exporting a layer, the projection should only contain the identifier, not the full object.
See screenshot
What do you think ?
Comment #16
augustus.kling CreditAttribution: augustus.kling commentedIt's time for the next iteration and some questions.
Things changed:
My questions / Open things:
I'm looking forward to any comments.
Comment #18
PolTesting right now !
Comment #19
PolIt 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.Layers' projections are exported as identifiersOk.There are still tabs and or trailing spaces in the patch, we have to clean them, but it's not the most important.
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.
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.
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 !
Comment #20
augustus.kling CreditAttribution: augustus.kling commentedAgain, 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.
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.
Comment #21
PolI made the Proj4JS module, I just did this in a hurry and I've set you maintainer.
Example of use:
You can also use
proj4js_load_definitions()
, who take an array as argument.Comment #22
augustus.kling CreditAttribution: augustus.kling commentedPol, thanks a lot for packing the proj4js module. I'll integrate it in the next patch iteration (sometime in the early upcoming week).
Comment #23
PolHello @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!
Comment #24
augustus.kling CreditAttribution: augustus.kling commentedPlease 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?
Comment #25
PolI made a boo boo sorry.
Comment #26
PolThis 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 !
Comment #27
augustus.kling CreditAttribution: augustus.kling commentedCan 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:
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.
Comment #28
PolI'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.
Comment #29
augustus.kling CreditAttribution: augustus.kling commentedIt 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.
Comment #30
PolHello 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 !
Comment #31
augustus.kling CreditAttribution: augustus.kling commentedThanks 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.
Comment #32
PolI see the problem now.
I tried to do:
But it doesn't work. I'll try to find a cleaner solution these days.
Comment #33
dianacastillo CreditAttribution: dianacastillo commentedI have time to do some testing if you guys need help, what do you want me to do?
Comment #34
augustus.kling CreditAttribution: augustus.kling commentedThanks 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.
Comment #35
PolHello 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.
Comment #36
augustus.kling CreditAttribution: augustus.kling commentedThe 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.
Comment #37
PolPerfect it works indeed :-)
Some remarks:
ctools_include('export');
missing in the functionopenlayers_get_projection_by_identifier()
.About that, shouldn't we use the same structure as in
openlayers_map_load()
?Quick example (not tested):
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.I'm thinking about a static variable, but I know that it's not a good pratice, any other idea ?
Thanks !
Comment #38
augustus.kling CreditAttribution: augustus.kling commentedThanks 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.
Comment #39
PolThat'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 !
What do you think if proj4js is a mandatory dependency of OpenLayers ?
Comment #40
augustus.kling CreditAttribution: augustus.kling commentedI 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.
Comment #41
PolYou'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: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.
I prefer the first solution, what do you think ?
Comment #42
PolHello 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!
Comment #43
augustus.kling CreditAttribution: augustus.kling commentedGiven 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.
Comment #44
augustus.kling CreditAttribution: augustus.kling commentedGiven 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.
Comment #45
PolPerfect :-)
Comment #46
augustus.kling CreditAttribution: augustus.kling commentedThe patch is now in. Please report any issues and things that require clarification so that proper and understandable documentation can be written.
Comment #47
augustus.kling CreditAttribution: augustus.kling commentedA documentation draft is now available at http://drupal.org/node/1939470.
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedHello,
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()
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedIt seems that the object is named 900913 in place of EPSG:900913.
Comment #50
augustus.kling CreditAttribution: augustus.kling commentedCan 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?
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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 ?
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedI just did a test without olfp and proximity. Nothing changes.
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commentedSo, I think I will go back to the last stable version for now on.
Comment #54
PolThis 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.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedI did the db upgrade and Proj4JS is installed as Libraries but the problem is still there.
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedSometimes 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
Comment #57
PolThen the problem comes from a module who provides a layer who doesn't use the new syntax, do you have geofield installed ?
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, I have. And views_geojson.
Comment #59
PolTry to disable them for now and see if you still have the problem.
Comment #60
Anonymous (not verified) CreditAttribution: Anonymous commentedIt seems working without geofield. But, I need it, what can I do ?
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedI have the 7.x-2.0-alpha2 version for geofield. Do I have to upgrade or go back to the 7.x-1 branch ?
Comment #62
PolIn the layer type class and/or map definitions, replace '900913' by 'EPSG:900913', it should work afterwards.
Comment #63
PolIn the file geofield.openlayers.inc,
replace all occurence of :
with
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm sorry I didn't understand very well the last post. Where is that layer type class ?
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedIt works now.
Thanks a lot.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedI will address that issue to geofield for them to modify in the dev.
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedHello,
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.
Comment #68
PolHello Augustus,
I think we should remove the description from the checkboxes, what do you think ? See the screenshot.
Thanks!
Comment #69
augustus.kling CreditAttribution: augustus.kling commentedYou'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?
Comment #70
PolI let you take care of that.
If you want, you can also commit the changes on the dclondon branch too !
Thanks!
Comment #71
augustus.kling CreditAttribution: augustus.kling commentedThe duplicated labels have been removed in a6bd2cd9d994ede2537798f16df5e7f81b69ebdf.
Comment #72
Rob C CreditAttribution: Rob C commented@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.
Comment #73
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commentedOk 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 ?
Comment #75
PolHi 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:
Comment #76
augustus.kling CreditAttribution: augustus.kling commentedI 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 itsdescription
column and therefore requires every layer to have an explicitdescription
.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 thesrs
orprojection
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
inopenlayers.install
line 523 when the error strikes. If you can't, dump the states of$layer
and watch for a layer that lacks adescription
property or has it set toNULL
.Fixing the issue would then mean setting a
description
property for the layer or change the table definition so that thedescription
column carries the empty string as default value.Comment #77
Anonymous (not verified) CreditAttribution: Anonymous commentedEach time I upgrade I have the errors I posted before concerning sanity_check.
Can you please post something to heal that ?
Thanks.
Comment #78
PolHello 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()
: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.
Comment #79
augustus.kling CreditAttribution: augustus.kling commentedLet 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 usearray_shift
because it modifies the passed array.Comment #80
PolHi Augustus,
I agree.
How to reproduce it, I just found an easy way to do it:
Comment #81
augustus.kling CreditAttribution: augustus.kling commentedIt 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?
Comment #82
PolI 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 ?
Comment #83
augustus.kling CreditAttribution: augustus.kling commentedThe 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 ownoptions_form_submit
implementation.Whilst applying the changes I noticed that
openlayers_layers_ui
'sedit_form_submit
checks for the existence of aoptions_form_submit
method before calling it. I would say the check is not necessary as the method is defined inopenlayers_layer_type
and therefore available for all layers. Is there something I'm overlooking?Comment #84
PolGood, thanks Augustus!
Are you talking about this bit of code in modules/openlayers_ui/plugins/export_ui/openlayers_layers_ui.class.php ?
Indeed this can be rewritten correctly now.
Comment #85
augustus.kling CreditAttribution: augustus.kling commentedThanks, I've removed the obsolete check in 6df04b79c1dcfe7922c11d9c418a3953ffcea120.
Comment #86
Maksym Kozub CreditAttribution: Maksym Kozub commentedI 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.
Comment #87
augustus.kling CreditAttribution: augustus.kling commentedThe 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:
openlayers_layers
table.geofield
, editgeofield.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.
Comment #88
Maksym Kozub CreditAttribution: Maksym Kozub commentedUpgraded 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:
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...
Comment #89
PolI'm working on it, I'm also having the problem now.
Comment #90
RdeBoerHi 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 ....
Comment #91
PolHello 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.
Comment #92
PolI'll close it for now, assuming that it's closed.