[2.2] Reworked gallery.phtml and include unit tests

[gwharton]

Description

This is dependent on PR #17969

Reworked gallery.phtml for gallery options and gallery fullscreen options.
Include unit tests

Fixed Issues (if relevant)

1. [2.2.4] Gallery theme variables being ignored #15009: 2.2.4 Gallery theme variables being ignored

Although Issue #15009 is marked as fixed, the changes committed to fix the issue have gone missing!!! and do not appear in 2.2-develop. This fixes the issue again (albeit in a better way than before!)

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[magento-engcom-team]

Hi @gwharton. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[gwharton]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @gwharton. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @gwharton, here is your new Magento instance.
Admin access: https://pr-17920.engcom.dev.magento.com/admin
Login: admin Password: 123123q

[miguelbalparda]

@gwharton thank you for noticing #15020 is gone. @dmanners can you please check?

[dmanners]

Hi @gwharton and @miguelbalparda yup looks like something was merged without the latest version of the file, it's easy to do if you are not keeping the branch up to date. If you are happy to continue @miguelbalparda then I would just go ahead and process this PR as normal.

Sorry for the inconvenience.

[tmotyl]

Hi
Thanks @gwharton for taking care. However just by reading i'm pretty sure the code will not work when setting false in view.xml like <var name="caption">false</var>.
The problem is a bad design of the $block->getVar() method which return the same result (bolean false) for not set var and var set explicitly to false in view.xml.
In my opinion the getVar() should return null on not set and value of the var when set.

Without changing getVar right now, this can be worked around just by removing the if statements, so always setting all properties in phtml.

[gwharton]

Yeah. I was never a fan of that update to getVar. I might look at it again. I may also look at moving the majority of the template code into a block function to facilitate unit testing.

[gwharton]

ok, on reflection, this is still a bit of a messy area. Please hold on this PR as I will be looking at this again and proposing further changes.

[gwharton]

@tmotyl @miguelbalparda If everyone agrees that this is the best way forward, I will proceed and generate unit tests to cover all view.xml options.

[gwharton]

Infact, I'm still not happy with this at all. I really don't like the change that was made to the getVar function back in #12285 to return boolean in the case of true/false and strings for everything else. It's leading to all sorts of problems determining whether variables are valid or not, and having to deal with different return types, and variables with mixed types depending on values.

I am going to revert that change, making it return false if the variable doesn't exist, or a string of the variable contents if it does exist. I will then implement an alternative fix for the original issue #12285.

This will mean that all variables will be strings, and they can be typed appropriately by code in the gallery template and block functions when output as json.

[gwharton]

@tmotyl @miguelbalparda OK, I think I have done just about as much as I would like to do on this one.

To summarise what I have done.

• Reverted change made to getVar function (as part of resolution to The option <var name="allowfullscreen">false</var> for mobile device don't work in product view page gallery #12285) (well actually one of the dependencies of getVar was changed) to restore it to always returning the variable contents as a string, instead of a mixture of strings and booleans, and false if the variable was not found.
• Removed unnecessary integration tests (no longer needed or replaced by integration tests)
• Reworked the gallery options and gallery fullscreen options sections of the gallery.phtml template by moving the html generation into their own block functions.
• Added new variables thumbmargin and keyboard to fullscreen gallery options.
• Implemented new fix to getBreakpoints function to ensure any true/false values are converted to boolean before json formatting. (ensures The option <var name="allowfullscreen">false</var> for mobile device don't work in product view page gallery #12285 remains fixed)
• Implemented unit tests to cover view.xml options

Points to think about for the future

• Should the blank and luma themes have default values added for all possible configuration options. I believe it is missing certainly thumbmargin and also possibly navarrows.
• This still leaves the gallery options and fullscreen options limited in terms of which fotorama options we support. If any additional fotorama variables are included in the view.xml they will still be ignored. This is unlike the getBreakpoints function, which will import all options it finds in view.xml.
• Docs require updating to cover all supported options.

[miguelbalparda]

I think this is way to big to be merged all at once. How about dividing this into a couple of PRs?

[gwharton]

ok, understood. I will split the reversion of #12285 and the new resolution to that issue into a new PR and leave the rest in this one to see if that looks more manageable.

[miguelbalparda]

Million thanks @gwharton! Feel free to assign the new PR to me and I'll give this a check.

[gwharton]

new PR #17969 created to cover the alternative fix to #12285

This PR updated to only include the gallery.phtml rework

[gwharton]

Once PR #17969 is agreed and merged, this will make this PR fail due to conflicts in the gallery.phtml. Once #17696 is merged I will resolve those conflicts in this PR. This should not be progressed until #17969 is agreed upon.