DPDBeNeLux / magento-DPD_Shipping

Let op: er zijn nieuwe DPD plugins (BETA versie) beschikbaar met belangrijke nieuwe functionaliteiten. De oude plugins zullen na Brexit niet meer goed functioneren, download daarom de nieuwe versie op Github/DPDconnect.
11 stars 10 forks source link

'gmapsapi' blockname is too generic #49

Open hostep opened 8 years ago

hostep commented 8 years ago

Hi

We noticed a block name conflict with another module: Bpost ShippingManager.

Both of the modules use the blockname gmapsapi in the frontend, which in turn causes to only one of them being used (in this particular case DPD as it comes alphabetically after Bpost). This is defined over here in the DPD module: https://github.com/DPDBeNeLux/magento-DPD_Shipping/blob/8216ce0bd1f6358f39dffca40595cd4af4506aaa/app/design/frontend/base/default/layout/dpd_shipping.xml#L23

My suggestion is to change the blockname to dpd.gmapsapi or something like that?

(This won't solve all the problems when having installed both these modules, I'm still looking into resolving other problems, if I find other things we can apply to this module to reduce the problems, I'll open a new PR or issue in the coming days)

mvgucht commented 8 years ago

Hey hostep,

can you give me some pointers on how to identify the block name conflict you see.

If we rename the block I presume that both of the modules want to load the gmapsapi. This is going to throw some google errors in the js console. (eg: maps already defined)

the Bpost one loads more libraries then ours.

   <script src="<?php echo $protocol;?>://maps.googleapis.com/maps/api/js?sensor=false&v=3.17&sensor=false&libraries=geometry,places" type="text/javascript"></script>

vs

<script src="https://maps.googleapis.com/maps/api/js?sensor=false&v=3.13" type="text/javascript"></script>

so next to changing the block name we would need to alter the template to check if 'maps' is already defined. If it is we don't add the script. and then we hope that the 3.17 version is compatible with our locator :)

hostep commented 8 years ago

Yes indeed, that's exactly what we discovered, but I don't know if it is the responsibility of the DPD module to provide a workaround for conflicts with the Bpost module. I think this is more a responsibility for integrators?

The problem with both modules using the same block name, is it take a while for an integrator to find the problem. In this case the google maps wasn't showing in the checkout for the bpost shipping methods and some confusing javascript error was thrown in the console.

If the block name is different, the gmaps js files will be included twice indeed, but integrators will quickly see the notice in the console about the two modules each trying to include those javascript files. Which is an improvement compared to the current situation I think. As you'll find the problem quicker. So as an integrator, next thing you'll have to figure out, is what to do with that problem (in our case, we removed the gmaps js files from the DPD module which seems to have solved most of the issues, not all of them though, in the next couple of days I'll take a closer look at the other issues).

There is not really a disadvantage to changing the blockname in the DPD module, it will actually allow developers to find the problem quicker.

I'll try to keep you updated in the next couple of days about this :)

hostep commented 8 years ago

@mvgucht maybe you could get in contact with the developers of the bpost module (which are actually the same as the original devs of the DPD module), to ask how they see these two modules working together?

mvgucht commented 8 years ago

I know the developers, If we ask they could make it 'compatible', but we'll have to do this for every other shipment module that we encounter the problem with.

But there is an other way, I started working on a framework that I could reuse on other php platforms. I had to stop working on it a couple of months agon, but I hope to pick up where I left of soon. The library (or a first attempt) is available here: https://github.com/dpdgroup/php-Library_Framework

and I'm looking to get the virtual machine running where I started to integrate the framework into magento so I can get the files of it.

the framework, once integrated in for example magento, prestashop, ... should enable carriers to use their TSF.library on all of the integrated platforms. And more, all of the carriers would use the same front-end integration.

hostep commented 8 years ago

Ok, so I got them both working simultaneously.

In the xml of the theme package, I added this:

    <!-- fix for conflicting blocknames between dpd & bpost -->
    <checkout_onepage_index>
        <remove name="gmapsapi"/><!-- remove the conflicting block name -->
        <reference name="head">
            <!--
            The DPD module is actually compatible with the gmaps js files which the bpost module includes,
            so we don't need to load the gmaps js files from the DPD module

            <block type="core/template" name="dpd.gmapsapi" template="dpd/gmapsapi.phtml"/>
            -->
            <block type="core/template" name="bpost.shm.gmapsapi" template="bpost/shm/gmapsapi.phtml"/>
        </reference>
    </checkout_onepage_index>
    <!-- end fix -->

And in the bpost module js/bpost/shm/checkout.js file, I changed this:


             //reset saturday selection
-            $('bpost-saturday').checked = false;
+            if ($('bpost-saturday') !== null)
+            {
+                $('bpost-saturday').checked = false;
+            }

             //remove error messages when switching
     deliveryDate: function (target) {
-        $("bpostDelivery").hide();
+        if ($("bpostDelivery") !== null)
+        {
+            $("bpostDelivery").hide();
+        }

         if(this.settings.datepicker_display) {

This seems to have solved all the problems we were experiencing.

Maybe for a real solution, the DPD module can add a setting in the backend, where you can choose with a yes/no to include the google maps javascript files. A lot of other modules work like that when they come with the jquery library, since lots of modules do this, it would be a disaster if lots of jquery libraries where included on one page, so most of the modules added a switch to enable the including of jquery with a yes/no option. So you could implement a similar solution to get around this problem of multiple modules including the google maps javascript files. What do you think?