Support from Acquia helps fund testing for Drupal Acquia logo

Comments

podarok’s picture

podarok’s picture

Issue tags: -Needs manual testing
FileSize
571.02 KB

here is a screenshot for 7.x-1.x release

podarok’s picture

Status: Needs review » Active

tag
@todo - fix relative issues

johnv’s picture

Some test-remarks:

  1. Patch #0 adds https:// support for the googlemaps files.
    It does this also for https://ditu.google.cn/maps/api/js?v=3&language=en&sensor=false
    As it is a Chinese version, no https is supported. Only thát link uses hardcoded http://
  2. "Warning: you have included the Google Maps API multiple times on this page. This may cause unexpected errors. "
    A bunch of .js files is removed from gmap_gmap, since they are already always loaded in _gmap_base_js()/hook_element_info().
    This makes the function _gmap_doheader() superfluous, too.
    Also, all javascript is now added via Form API, not drupal_add_js.
  3. The location of gmap_markers still is a PITA. On my installation, the current URL $GLOBALS['base_url'] . '/' . variable_get('file_public_path', conf_path() . '/files') . '/js/gmap_markers.js' breaks my site: the result of this call is http:// , but my browser fetches https://, giving an red error:
    GET https://example.com:8082/sites/all/files/js/gmap_markers.js
    So, I set it back to '/' . variable_get('file_public_path', conf_path() . '/files') . '/js/gmap_markers.js' , which makes me happy :-)
    @podarok, this may upset someone, or you. But let's only change this for documented cases (http vs https, subdomains...
  4. Function gmap_gmap($op, &$map) , case '$op == pre_theme_map' contains a sections that adds js.
    This is removed/moved, since this functionality is already in the 'official' hook _gmap_pre_render_map() and callback _gmap_base_js .
  5. Function _gmap_doheader() - which adds js-files - is removed, since this functionality is done in the 2 mentioned functions. I moved the relevant parts to _gmap_base_js() . Perhaps it should be in pre-render().
  6. The hook gmap_views_pre_render() has no use, since its only function is to add a js-file.
    I removed it and added 'gmap_views_ajax.js' via _gmap_base_js().

Some things must still be tested:
- Two displays on the same page with different markertype (e.g, cluster vs. non-cluster)

podarok’s picture

#4 please, provide a patch for that ))))

johnv’s picture

Status: Needs work » Active
Issue tags: -Needs manual testing, -API change, -Needs change record, -Needs dependency review

Yes, I'll create a patch.
First, I will log my errors on testing, editing #4 to not create a long list of comments.

johnv’s picture

Status: Active » Needs review
FileSize
10.88 KB

Please find attached patch.
IMO only #1167594: Saving gmap_markers.js needs catch errors remains.
I concentrated on two things:
- centralize addition of javascript, in the 2 'official' hooks/callbacks: _gmap_base_js() and _gmap_pre_render_map().
- remove unused code. I merely uncommented this, so other people can easily see what is removed and why. We should get rid of these parts as soon as the version stabilizes.
- the variable_get('file_public_path' ... issue. It works on my site. I invite others to test on theirs and report the findings, together with their constellation.

podarok’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing
+++ b/gmap.moduleundefined
@@ -129,7 +131,10 @@ function gmap_gmap($op, &$map) {
+// The following required js-files are already attached in  _gmap_pre_render_map() / _gmap_base_js(). ¶
...
@@ -138,8 +143,10 @@ function gmap_gmap($op, &$map) {

trailing whitespace http://drupal.org/coding-standards#indenting
Another issue for creating such patches - http://drupal.org/node/1354#code if You commenting a code - You should use or code samples or @todo with rer line comments for easy patch reviewing

Bad example

/*
 code
 code
 code
*/

Preffered style

// code
// code
// code

It is still needs work
We shouldn`t commit any part of code with comments and debugs instead of the @todo http://drupal.org/node/1354#todo with links to real issues or @deprecated with links to real change-records.
But this patch is good enouph for a Needs manual testing, so tagging

johnv’s picture

So, what do you prefer? Shall I remove all obsolete code?

johnv’s picture

A clean patch.

johnv’s picture

Status: Needs work » Needs review

testbot.

Status: Needs review » Needs work

The last submitted patch, gmap2_1931138_10_fix_all_markers.patch, failed testing.

johnv’s picture

Status: Needs work » Needs review
FileSize
10.75 KB

new version.

drtrueblue’s picture

Here is another marker issue that was closed, won't fix. It was requested that I retest the issue with the new version. Unfortunately, it is still not resolved. My apologies if this should be a separate issue from this current thread, I was trying to help concentrate the same type of issues. Thanks for your review and support.

https://drupal.org/node/1884604

johnv’s picture

Issue summary: View changes

Updated issue summary. Added Related

johnv’s picture

@drtrueblue, no, it is not resolved yet in the latest -dev version (1.x nor 2.x). But it is after applying this patch.
I did a test for a view with locations on a related node/entity, and the view was OK.
So can you check your case, and report back? :
- install 7.x-2.x, apply this patch #1931138-13, flush caches.

podarok’s picture

Issue summary: View changes

Updated issue summary.

podarok’s picture

Issue summary: View changes

Updated issue summary.

podarok’s picture

Issue tags: -API change
+++ b/gmap.moduleundefined
@@ -349,7 +276,8 @@ function _gmap_base_js() {
-  $ret[$GLOBALS['base_url'] .'/'. variable_get('file_public_path', conf_path() . '/files') . '/js/gmap_markers.js'] = array(
+  $ret['/' . variable_get('file_public_path', conf_path() . '/files') . '/js/gmap_markers.js'] = array(
+//  $ret[$GLOBALS['base_url'] . '/' . variable_get('file_public_path', conf_path() . '/files') . '/js/gmap_markers.js'] = array(

we shouldn`t leave such comments with code
If You want to be descriptive, just type a small description why this changed in comment with patch or(and) in function phpdoc if this is usefull for DX
#13 looks good
but we still needs tests for that and #1931138-20: Fixing all 'single marker' issues

the test should include (updating summary)

  1. 2 testing views with gmap style plugins
  2. testing all js files for access (http status 200) on view page
  3. testing views plugins forms via nojs get requests
podarok’s picture

Status: Needs review » Needs work
Issue tags: +API change

Shall I remove all obsolete code?

Yes - preffered style - removing obsolete code
After pushing such patch to repo we should create change-record with
before
old code example
after
new code example
examples

PS.tag API change

johnv’s picture

Hi podarok, unfortunately, I cannot help you with automated tests.

podarok’s picture

#18
I`d planned this for myself with examples for You or someone else
With tests we can much easier implement new features and fix bugs without generating new ones
It`s easy...

podarok’s picture

Issue tags: -Needs dependency review

BTW important addition for #13 review
if we removing the function we should leave wrapper for minor release update dependency stability

+++ b/gmap.moduleundefined
@@ -358,62 +286,6 @@ function _gmap_base_js() {
- */
-function _gmap_doheader() {

needs work

function _gmap_doheader() should be leaved with wrapper code and @deprecated in comments
Example

/**
 * @deprecated will be removed in 7.x-(X+1).x release 
 * @see http://path/to/issue/with deprecated patch
 * @see http://path/to/change-record with example before/after
 * @todo remove this before http://path/to/7.x-(X+1).x-release-roadmap-issue fixed
 */
function _gmap_doheader() {}

This is easy and useful for those modules and especially tests!!! that possibly used this function and do not brakes their functionality in minor gmap release update

podarok’s picture

Issue tags: +API change, +Needs dependency review

tag

johnv’s picture

Regarding the deletion of _gmap_doheader():
IMO functions with a preceding '_' are supposed to be internal functions, and not to be used by other modules.
But indeed, better be cautious, to not break other modules.
We might add a watchdog message, to give some warning.

podarok’s picture

Issue tags: +Needs dependency review

MO functions with a preceding '_' are supposed to be internal functions, and not to be used by other modules.

Yes, this is true
But, due to bad code coverage with tests and partially abandoned(400+ bug issues)/bad-api-descripted gmap module before @podarok->maintainer we should be painless for those developers that do reverse engineering and integrated their work with current implementation

podarok’s picture

We might add a watchdog message, to give some warning.

nope, @deprecated + change-record is a good workflow
these functions will be highlighted as strike in most IDE

podarok’s picture

Issue tags: +Needs change record

tag

podarok’s picture

Status: Active » Needs review
Issue tags: +Needs manual testing, +API change, +Needs change record, +Needs dependency review
podarok’s picture

Status: Needs review » Needs work

patch pass tests, but needs reroll due to #16 and #20

podarok’s picture

Issue summary: View changes

Updated issue summary.

johnv’s picture

Status: Needs work » Needs review
FileSize
10.42 KB

Attached patch is a reroll of #13, including #16.
You might want to add the documentation in #20 yourself.

johnv’s picture

new try.

podarok’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs dependency review +Needs documentation, +RTBC for 7.x-2.5

#29 commited pushed to 7.x-2.x-dev
tagged

podarok’s picture

Status: Needs work » Needs review
FileSize
630 bytes

some docs and DX info

podarok’s picture

+++ b/gmap.moduleundefined
@@ -285,9 +285,10 @@ function _gmap_base_js() {
+ * @deprecated will be removed in next major release upgrade ¶

trailing whitespace will be removed before commit))

podarok’s picture

Component: Meta issues » Code
Assigned: podarok » Unassigned
Priority: Major » Normal
Status: Needs review » Needs work
Issue tags: -Needs documentation

#31 commited pushed to 7.x-2.x

drtrueblue’s picture

Thank you for the time and effort. I apologize for my somewhat novice experience level, but I'm not skilled at applying patches. It's reassuring to know you've put forth a good fix that you've tested and I'll patiently await the next formal update. Again, thank you for the consideration.

podarok’s picture

#34 this patch already commited
You can try 7.x-2.x-dev release

podarok’s picture

Issue summary: View changes

Updated issue summary.

johnv’s picture

This patch needs a follow-up to use the correct URL's:
https://developers.google.com/maps/documentation/geocoding/#GeocodingReq...

mapee’s picture

Version: 7.x-2.x-dev » 7.x-2.5-beta1
Component: Code » Meta issues
Category: task » bug
Priority: Normal » Major

hello,

i updated gmap from v. 7. 2.4 to v. 7. 2.5 beta1, but all markers not showing up ( the map become empty) i uploaded markermanager.js and markermanager_packed.js to thirdparty folder
any help?? iam using GMaps Utility Library MarkerManager

natureofdorset’s picture

I have just upgraded to 7.2.4 and my markers no longer display. I went further and uploaded 7.2.5-beta1 but no change. The log file shows 'page not found' and default/files/js/gmap_markers.js

Help, please, asap!

edvanleeuwen’s picture

The log file shows 'page not found' and default/files/js/gmap_markers.js

It might be that the wrong path is used. See http://drupal.org/node/1937990. To test: look into your log report and verify the path with the setting. If those differ, you could revert to a previous version or wait until this issue is solved.

podarok’s picture

Version: 7.x-2.5-beta1 » 7.x-2.5-beta2
Category: bug » task

now this is a latest pre-stable version

mapee’s picture

Version: 7.x-2.5-beta2 » 7.x-2.6
Category: task » bug

same problem on gmap 2.6 - drupal 7 , no markers dispaly, please any help,
but on the wrong version (2.5) it works

johnv’s picture

@mapee, please check your Browser developer tools, like Chrome | Extra | Developer tools, and check for (javascript) errors.

mapee’s picture

thanks johnv

i tried for many browsers like chrome , firefox , safari , internet explorer, in the end the same problem,

you can see this link for multiple markers
http://www.mapee.net/new/

for single marker:
http://www.mapee.net/new/node/221

i am using gmap extended
‏Data Source: choose Latitude and Longitude fields

johnv’s picture

@mapee, I mean this messag in the Developer tools:
GET http://www.mapee.net/sites/default/files/js/gmap_markers.js 404 (Not Found)
Does the fiel exist on the server? Did you generate the markers, using admin/config/services/gmap?

mapee’s picture

thank you very much johnv,

as you said, the problem was gmap_markers.js (Not Found)

i correct it.

thanks again

podarok’s picture

Version: 7.x-2.6 » 7.x-2.7-beta1
Priority: Major » Normal
Status: Needs work » Fixed

looks like all are fixed
feel free to reopen if not

johnv’s picture

Title: Fixing all markers issues » Fixing all 'single marker' issues

Changing title, since the following set of issues will be "Fixing all Clustering issues" (due to APIv2 -> APIv3 conversion), like:
#1954304: MarkerClusterer not work, give JS error.
#1968416: Markers disappear when using Jef Poskanzer's Clusterer (due to Google API v3)

Status: Fixed » Closed (fixed)

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

Fly85’s picture

Status: Closed (fixed) » Active

Hi, I have the same problem with gmap 7.x-2.6, I changed to 7.x-2.7-rc1+3-dev and still have no markers displayed.
I have one javascript error :

Warning: you have included the Google Maps API multiple times on this page. This may cause unexpected errors.

I used the code from the help folder to display a map in a node. From my template.php :

  $map_array2 = array(
    'id' => 'example',
    'maptype' => 'Terrain',
    'width' => '400px',
    'height' => '400px',
    'latitude' => 12.345,
    'longitude' => 12.345,
    'zoom' => 4,
    'align' => 'left',
    'controltype' => 'Small',
    'mtc' => 'standard',

    'behavior' => array(
      'locpick' => FALSE,
      'nodrag' => FALSE,
      'nokeyboard' => TRUE,
      'overview' => TRUE,
      'scale' => TRUE,
    ),

    'markers' => array(
      array(
        'text' => 'First Marker',
        'longitude' => 12.345,
        'latitude' => 12.345,
        'markername' => 'lblue',
      ),
    ),
  );

  $element2 = array(
    '#type' => 'gmap',
    '#gmap_settings' => $map_array2,
  );
  
  $output2 = drupal_render($element2);
  $variables['map'] = $output2;

The map is displayed but there are no markers.

-- edit --

By adding

  'options' => array()

in the marker array fixed it. Sorry for reopening the ticket.

johnv’s picture

@Fly85, so you are programmatically adding a map? If so, please open a new issue. Perhaps the 'code from the help folder' is outdated.

Garra’s picture

Version: 7.x-2.7-beta1 » 7.x-2.7

Hello
i have problem to display marker

The marker is displaying when the user select the addresse directly in the map
but it does not display when VIEW node.

I have 7.x-3.0-rc3 version of location and 7.x-2.7 of gmap (7.23 Drupal version)

i'd tested with location as field and with location in node locations.
both the same problem

could anybody help me???

thanks a lot

drupalina’s picture

Garra, I have been struggling with this issue in D7 for many months (probably more than a year). see this for example: https://drupal.org/node/1552238

Have you found any solution to get this to work?

Does anybody know how to make GMap display the pinpoint from the address provided in Location field in D7?

kenorb’s picture

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

The same problem with Dev version.
This file exists in: ./sites/default/files/gmap_markers.js
But gmap is looking in sites/default/files/js/gmap_markers.js

kenorb’s picture

Status: Needs work » Active
Issue tags: -Needs tests +RTBC for 7.x-2.5

It happens especially during the upgrade of gmap module, so the previous file probably was in different place and the new file is suppose to be in public://js/gmap_markers.js
But file_save_data($contents, 'public://js/gmap_markers.js', FILE_EXISTS_REPLACE); works like this, if the file exists, then it points to the existing path which was generated before.
And checking this line:

$ret[base_path() . variable_get('file_public_path', conf_path() . '/files') . '/js/gmap_markers.js'] = array(

it seems the path is hardcoded despite of saved gmap_marker_file fid.
I think we should check and load the existing filepath like:

$filepath = ($fid = variable_get('gmap_marker_file', NULL)) ? file_load(variable_get('gmap_marker_file', NULL))->uri : 'public://js/gmap_markers.js';

WORKAROUND

For workaround, I'm suggesting:

drush eval "file_unmanaged_delete(variable_get('gmap_marker_file', NULL)->uri);"
drush -y vdel gmap_marker_file

And regenerate gmap markers at /admin/config/services/gmap or by clearing the cache from UI (as in CLI it doesn't work).

If you'll have still some problems, probably you have more than one file for gmap markers. You can see them all by the following command:

drush sqlq "SELECT fid, filename FROM file_managed WHERE status = 1 AND filename LIKE 'gmap_markers%'"

You can remove these file by (ignore the first errors):

drush sqlq "SELECT fid FROM file_managed WHERE status = 1 AND filename LIKE 'gmap_markers%'" | xargs -L1 -I% drush -v eval "file_delete(file_load(%));"
kenorb’s picture

Issue summary: View changes

Updated issue summary.

kenorb’s picture

drupalina’s picture

For whatever it's worth, I got the markers to show by replacing the entire gmap-2.7 with the latest .dev version.

kenorb’s picture

Status: Active » Needs review
FileSize
601 bytes
podarok’s picture

Good to see here simpletest test

kenorb’s picture

Status: Needs review » Needs work

Few issues I've found with the #57 (like all to a member function getExternalUrl()), so my better proposal is:

--- gmap.module
+++ gmap.module
@@ -284,8 +284,14 @@ function _gmap_base_js() {
     'type' => 'external',
     'weight' => 1,
   );
+
   $uri = ($fid = variable_get('gmap_marker_file', NULL)) ? file_load(variable_get('gmap_marker_file', NULL))->uri : 'public://js/gmap_markers.js';
-  $ret[file_stream_wrapper_get_instance_by_uri($uri)->getExternalUrl()] = array(
+  if ($wrapper = file_stream_wrapper_get_instance_by_uri($uri)) {
+    $fullpath = $wrapper->getExternalUrl();
+  } else {
+    $fullpath = base_path() . variable_get('file_public_path', conf_path() . '/files') . '/js/gmap_markers.js';
+  }
+  $ret[$fullpath] = array(

based on the patch code.
I'll provide updated patch soon.

hutch’s picture

The directory "files/js" is used by Drupal's javascript aggregation which has in my experience deleted everything in there when being reset or unset, although gmap_markers.js will be rebuilt at the next cron job it is possible for gmap to find itself without a markers file for a while. The solution in my opinion is to store gmap_markers.js in its own directory under files, eg "files/gmap/gmap_markers.js"

kenorb’s picture

Status: Needs work » Needs review
FileSize
792 bytes
podarok’s picture

Status: Needs review » Needs work

I can`t push it without testing coverage

kenorb’s picture

Marked as duplicated of this: #2086529: Location marker is not shown

jami3z’s picture

When applying #61, I get the following error when trying to regenerate the marker cache

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'public://js/gmap_markers.js' for key 'uri': INSERT INTO {file_managed} (uid, filename, uri, filemime, filesize, status, timestamp, type) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7); Array ( [:db_insert_placeholder_0] => 2 [:db_insert_placeholder_1] => gmap_markers.js [:db_insert_placeholder_2] => public://js/gmap_markers.js [:db_insert_placeholder_3] => application/x-javascript [:db_insert_placeholder_4] => 4340 [:db_insert_placeholder_5] => 1 [:db_insert_placeholder_6] => 1385349684 [:db_insert_placeholder_7] => undefined ) in drupal_write_record() (line 7154 of /xxxxxxxx/docroot/includes/common.inc).

podarok’s picture

Component: Meta issues » Code
Issue summary: View changes
Status: Active » Needs work

due to #64 the patch #61 needs work

kenorb’s picture

spidersilk’s picture

I applied the patch from #61 a few months ago, along with the one from #2185395: Use managed file table to retrieve gmap_markers.js, because it took both to get the markers showing. And everything seemed fine... Until today, when I got the error in #64.

But I didn't get it while trying to specifically do anything to the marker cache, or even while doing anything involving Gmap at all - it happened when I was trying to save changes from the Modules page (installing a new module, unrelated to Gmap). And there were a few other oddities - the duplicate URI referred to was 'public://gmap_markers_942.js', and when I checked the files directory, it looked like the markers file had been replicating itself over and over, so that there were now 1003 copies of it! With corresponding entries in the files_managed table. I did file comparisons between a few of them, and they were all identical. 1003 copies of the same file!

I have no idea why it was trying to insert version 942 a second time, or why it was trying to do anything with the map markers on an admin page that had nothing to do with the maps, but it seems like it might be related to this patch... Though the site is also using Media Browser Plus, in case that's relevant.

Another oddity: while there is a folder called gmap inside the files folder, these files aren't being generated there - they're just in the files folder. The gmap folder is empty.

Any ideas?

SocialNicheGuru’s picture

I would like to add private file system markers to the Meta list:
https://www.drupal.org/node/1940996

jomarocas’s picture

hi i update and issue that the status is active

jomarocas’s picture

Status: Needs work » Closed (works as designed)

Sorrty, work at expect if you choose lat and lon fields

kenorb’s picture

Status: Closed (works as designed) » Needs work