search

omise / omise-prestashop

Omise PrestaShop Plugin
https://docs.opn.ooo/prestashop-plugin
MIT License
4 stars 7 forks source link

Dual version compatibility #62

Closed jonrandy closed 6 years ago

jonrandy commented 6 years ago

1. Objective

This PR reinstates compatibility with Prestashop 1.6, which appears to be far more popular than 1.7. It does not however, break 1.7 compatility - we now have a plugin that works on both versions. Merchants who use 1.6 will be able to enjoy all future (and past - there have been 2 releases without 1.6 compatibility) enhancements to the plugin.

2. Description of change

Essentially I compared the 2 versions, and wrote code that cater to either version in areas where version specific code is required. The changes were surprisingly not that widespread. Luckily, Prestashop had documented the changes from 1.6 -> 1.7 quite well here

3. Quality assurance

I tested using Bitnami's docker images for PrestaShop from here.

Tested with Prestshop versions 1.7.4 and 1.6.1.9

Details:

To test the plugin, you should set up a Prestashop instance for each version (1.6, 1.7), install the plugin, and check all plugin features are working. I recommend using the Docker method above - super easy

N.B. - there seems to be an issue with Prestashop 1.7 whereby the installer appears to never end. However, this does not appear to be our fault as I've tried installing a number of other plugins and this occurs with all of them. If you refresh the page after the installer appears to have hung, you will find the plugin has actually installed successfully. I've googled this issue, but as yet have found no resolution.

4. Impact of the change

Prestashop 1.6 users can now enjoy all the latest and greatest updates from us 😄

5. Priority of change

Normal

6. Additional notes

Go Croatia! 🇭🇷 ⚽️ 🏆

jonrandy commented 6 years ago

After we merge this, I think we can go for a new release v1.6

guzzilar commented 6 years ago

@jonrandy is it possible to check version at the class-loader place instead of adding that IS_VERSION_17 condition everywhere?

Could be a place like 👇 ? https://github.com/omise/omise-prestashop/blob/dual-version/omise/controllers/front/base.php#L6

And then maybe we can have to 2 classes to handle 2 version concerns separately.

- /omise/classes/1.6/omise_charge_class.php
- /omise/classes/1.7/omise_charge_class.php

More crazy idea is to have a new loader class to taking care of which files that needed to be loaded for a specific version.

jonrandy commented 6 years ago

Refactored to remove all those ifs

guzzilar commented 6 years ago

@jonrandy I'm sorry for Croatia but saw you also tweaked a bit on the inapplicable template, can you also add a screenshot at the PR's description?

Thanks.. and sorry for Croatia again.. 😢

jonrandy commented 6 years ago

@guzzilar There is no visible difference on either PrestaShop version that results from this tweak to the template

guzzilar commented 6 years ago

@jonrandy so this line is just for dual version compatibility? https://github.com/omise/omise-prestashop/pull/62/files#diff-374fd4fbc70b1795ff22dbb8ea4a8526R7

jonrandy commented 6 years ago

Yes - the template between the older version of the plugin and the newer one only had one header tag removed. DIdn't seem worth splitting into 2 templates so I just made the template optionally accept a title

jonrandy commented 6 years ago

Yes - the template between the older version of the plugin and the newer one only had one header tag removed. DIdn't seem worth splitting into 2 templates so I just made the template optionally accept a title

guzzilar commented 6 years ago

@jonrandy fyi, your text-editor messed up with that .xml file again lol (at the latest commit https://github.com/omise/omise-prestashop/pull/62/commits/28226172a152656a5cd752bd55704a8917472684)

jonrandy commented 6 years ago

It's not my text editor. PrestaShop itself is doing it