[Backport][2.2] Reworked gallery.phtml to move generation of gallery json strings to own block functions

[gwharton]

Original Pull Request

#18440

Description

Reworked gallery.phtml to move generation of gallery json strings to own block functions
Introduced unit tests to cover generation of gallery json strings with full type checking.

The resultant code is much easier to diagnose and test. It uses the json_encode and json_decode routines instead of the old manual way of generating json strings that were used in the old gallery.phtml which is prone to error and mistakes.

Fixed Issues (if relevant)

1. none. Rework and unit tests.

Manual testing scenarios

1. View source on any product page.
2. Ensure the gallery section has fully formed json for gallery and gallery fullscreen sections.

Stock Magento should be

"options": {"nav":"thumbs","loop":true,"keyboard":true,"arrows":true,"allowfullscreen":true,"showCaption":false,"width":700,"thumbwidth":88,"thumbheight":110,"height":700,"transitionduration":500,"transition":"slide","navarrows":true,"navtype":"slides","navdir":"horizontal"}

and

"fullscreen": "nav":"thumbs","loop":true,"navdir":"horizontal","navarrows":false,"navtype":"slides","arrows":true,"showCaption":false,"transitionduration":500,"transition":"slide"}

All strings/bools/ints are now typed properly as per the Magento dev docs for the Magento 2 Gallery Widget.

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)

[gwharton]

The 2.3 PR is here #18440.

This PR brings 2.2-develop up to the same status as 2.3. It includes rework of the gallery template file and inclusion of more extensive unit testing.

[magento-engcom-team]

Hi @sidolov, thank you for the review.
ENGCOM-3988 has been created to process this Pull Request

[ihor-sviziev]

Hi @gwharton,
I just compared this PR to original one, it contains little bit another change set.
For instance in file Magento/Catalog/Block/Product/View/Gallery.php we don't have some cleanups.
Could you add all of your changes to this pr from original one? It would be much easier to review equal changes.
Thank you!

[gwharton]

@ihor-sviziev No problems. I'll get those changes made later today.

A point of note which I hope you can clarify. I'm not sure where I heard this, but I believed it was recommended to use fully qualified class path names in 2.2, and use the "use" statement and shorthand class path names in 2.3. That was why I did not adjust the class path names in the 2.2 PR, hence the difference in the 2.2 and 2.3 PRs. I've tried searching for this advice again to confirm and cannot find it, and see that 2.3 tends to use the shorthand with "use" clause method for all new code. Should I assume this should be followed for all new code in both 2.2 and 2.3 branches, and under what circumstances class pathnames in existing code should be altered when modifying code?

[ihor-sviziev]

Hi @gwharton,

A point of note which I hope you can clarify. I'm not sure where I heard this, but I believed it was recommended to use fully qualified class path names in 2.2, and use the "use" statement and shorthand class path names in 2.3. That was why I did not adjust the class path names in the 2.2 PR, hence the difference in the 2.2 and 2.3 PRs. I've tried searching for this advice again to confirm and cannot find it, and see that 2.3 tends to use the shorthand with "use" clause method for all new code. Should I assume this should be followed for all new code in both 2.2 and 2.3 branches, and under what circumstances class pathnames in existing code should be altered when modifying code?

Thank you for raising it!
Will discuss it with another maintainers.

[gwharton]

I've updated the classnames in 2.2 to use the "use FQCN" formats in Magento/Catalog/Block/Product/View/Gallery.php so the same changes are applied in both branches, although to be honest, I could revert ALL changes to this file in both branches without breaking things (as all required code changes were moved from Gallery.php to GalleryOptions.php, the changes remaining in Gallery.php were mainly cleanup (although it does adjust the json encode function to use framework instead of php functions (totally unrelated to this fix, but good to have changed))).

The differences in Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php are because these two test file have diverged slightly between 2.2 and 2.3, but the PRs make the necessary changes in both branches, including the "use FQDN" format for class names in both branches.

The changes in file dev/tests/integration/testsuite/Magento/Catalog/Block/Product/View/GalleryTest.php are only present in 2.2 as this test was removed in 2.3.

[ihor-sviziev]

Hi @gwharton,
I got answer about using fully qualified class names from @orlangur :

this was never true, the only recommendation of using FQCN was for WebAPI interfaces, this is irrelevant for 2.3-develop after #17424 and still relevant for 2.2-develop

Thank you for updating PR, will review it soon

[orlangur]

Let's wait for 2.3 PR finalization and then rewrite history in this one.

[orlangur]

@gwharton please rewrite branch history similar to original PR.

[gwharton]

@orlangur History squashed and force pushed, as requested.

[magento-engcom-team]

Hi @sidolov, thank you for the review.
ENGCOM-3988 has been created to process this Pull Request

[VasylShvorak]

✔️ QA passed

[m2-assistant]

Hi @gwharton, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.