There is a javascript issue when trying to display a map with overlay, so the overlay is not shown.

Here is my map configuration:

$map_info['Esri-imagery-places'] = array(
    'label' => 'Esri Imagery + Places (zoom 0..17)',
    'description' => t('Esri Imagery + Places'),
    'settings' => $default_settings,
    'layers' => array(
      'base' => array(
        'layer_type' => 'base',
        'urlTemplate' => 'https://server.arcgisonline.com/ArcGIS/rest/services/World_Imagery/MapServer/tile/{z}/{y}/{x}',
        'options' => array(
          'attribution' => 'Esri',
        ),
      ),
      'overlay' => array(
        'layer_type' => 'overlay',
        'urlTemplate' => 'https://services.arcgisonline.com/ArcGIS/rest/services/Reference/World_Boundaries_and_Places/MapServer/tile/{z}/{y}/{x}',
        'options' => array(
        ),
      ),
    ),
  );

It fails with a javascript error
Cannot read property 'addLayer' of undefined

CommentFileSizeAuthor
#5 leaflet-overlay_js_issue-3153300-5.patch744 bytesitamair
#2 leaflet-overlay_js_issue-3153300-2.patch382 bytesvdsh

Comments

vdsh created an issue. See original summary.

vdsh’s picture

vdsh commented
Status: Active » Needs review
StatusFileSize
new leaflet-overlay_js_issue-3153300-2.patch382 bytes

Here is a patch fixing the js issue

itamair’s picture

itamair commented
Status: Needs review » Needs work

Yes @vdsh ... should be. This is an advanced feature, that I inherited from the first D8 scaffolding code.
Never really tested ...

Well written, Php/Drupal coding standards, and not regressive (!!!) patches are super welcome ... tnx!

vdsh’s picture

vdsh commented

Hi itamair,

I provided the patch which is quite small in #2. Did you set the status back to needs work on purpose? if yes, what is missing from the patch?

Thanks

itamair’s picture

itamair commented
Status: Needs work » Needs review
StatusFileSize
new leaflet-overlay_js_issue-3153300-5.patch744 bytes

My bad @vdsh ... I wrote my comment few seconds you posted your patch. Thanks.
I reviewed it.

Well ... the matter is that some of the other Drupal.Leaflet.prototype methods are relying on the Drupal.Leaflet[mapid] object, and also the Drupal.Leaflet.prototype.add_overlay that you patched (you can see the addOverlay method applied to it at #213, and so on ...).

The addLayer is probably not working before it is invoked before the specific Drupal.Leaflet[mapid] has been defined.
The attached patch instead moves the definition of Drupal.Leaflet[mapid] right after the definition of the specific mapid map,
so the Drupal.Leaflet.prototype.add_overlay should be working on it ... and apply to that specific mapid Leaflet (in case of multi Leaflet maps contexts).

I guess the leaflet.drupal.js is far to be perfect and clean, but I would prefer this latter patch, for sake of coherence with the other parts of it (I don't have time to better refactor all this js file).
Let me know if this works for you too and it doesn't rise any regression (for what you experience). I would go for this one ... if I am not wrong.

vdsh’s picture

vdsh commented

Hi itamair,

Again, thanks for reviewing and replying so quickly.

I tested your patch and it works well

I based mine on Drupal.Leaflet.prototype.add_base_layer where we also have self.lMap.addLayer(map_layer);
So I just reproduced the same code and it worked.

Your point

Drupal.Leaflet.prototype methods are relying on the Drupal.Leaflet[mapid]

makes sense though.

I counted very quickly, there are 13 Drupal.Leaflet[mapid].lMap occurences and 9 self.lMap occurences (but only 3 outside the initialise function). Maybe it makes sense to keep your patch and replace all occurences of self.lMap by Drupal.Leaflet[mapid].lMap?

itamair’s picture

itamair commented

Well ... I am not totally sure.
I initially inherited this js implementation from the first Leaflet D8 scaffold, with this prototype definition (that I never really fell in love with).

I introduced the Drupal.Leaflet[mapid].lMap to implement multi Leaflet map functionalities and point to specific one.
But (if I had time) I would try to implement better js closures in the code to accomplish the same, and provide a wide refactoring of the whole file.
That might become a new specific task. What you think?

For this specific task I push the #5 and resolve this for now ...

  • itamair committed 8504b3b on 8.x-1.x 
    Issue #3153300 by vdsh, itamair: Overlay not working
itamair’s picture

itamair commented
Status: Needs review » Fixed
vdsh’s picture

vdsh commented

Hi itamair,

Yes, agreed that it could be dealt with separately - and probably involve more work than just replace all occurences of Drupal.Leaflet[mapid].lMap by self.lMap

I was just a bit worried that the fact that you do Drupal.Leaflet[mapid].lMap = self.lMap; early and then do some actions on self.lMap would provide some inconsistencies between Drupal.Leaflet[mapid].lMap and self.lMap, but due to the way js is working, if you modify self.lMap.a, then it's modifying exactly Drupal.Leaflet[mapid].lMap.a so I think we are fine here.

Cheers

Status: Fixed » Closed (fixed)

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