From a48dd7cd8bbd938f8e05462dde96bdc3701acf98 Mon Sep 17 00:00:00 2001 From: gwharton Date: Tue, 4 Sep 2018 18:27:26 +0100 Subject: [PATCH 01/17] Reworked gallery.phtml Updated code to output gallery variables. If items are not present in config, they are no longer output. (apart from gallery/nav and gallery/fullscreen/nav. gallery/nav now support false/thumbs/dots Updated all booleans to check type Confirmed output compliant with Magento docs. Left gallery/thumbmargin in, even though not in docs --- .../templates/product/view/gallery.phtml | 112 +++++++++++------- 1 file changed, 68 insertions(+), 44 deletions(-) diff --git a/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml b/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml index b2fa8e9aaf80f..13ac0c9937718 100644 --- a/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml +++ b/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml @@ -44,77 +44,101 @@ "magnifierOpts": getMagnifier() ?>, "data": getGalleryImagesJson() ?>, "options": { - "nav": "escapeHtml($block->getVar("gallery/nav")) ?>", - getVar("gallery/loop"))) : ?> - "loop": escapeHtml($block->getVar("gallery/loop")) ?>, + getVar("gallery/nav"))) : ?> + "nav": getVar("gallery/nav") ? '"true"' : '"false"'; ?> + + "nav": "escapeHtml($block->getVar("gallery/nav")) ?>" - getVar("gallery/keyboard"))) : ?> - "keyboard": escapeHtml($block->getVar("gallery/keyboard")) ?>, + getVar("gallery/loop"))) : ?> + ,"loop": getVar("gallery/loop") ? 'true' : 'false'; ?> - getVar("gallery/arrows"))) : ?> - "arrows": escapeHtml($block->getVar("gallery/arrows")) ?>, + getVar("gallery/keyboard"))) : ?> + ,"keyboard": + getVar("gallery/keyboard") ? 'true' : 'false'; ?> - getVar("gallery/allowfullscreen"))) : ?> - "allowfullscreen": escapeHtml($block->getVar("gallery/allowfullscreen")) ?>, + getVar("gallery/arrows"))) : ?> + ,"arrows": getVar("gallery/arrows") ? 'true' : 'false'; ?> + + getVar("gallery/allowfullscreen"))) : ?> + ,"allowfullscreen": getVar("gallery/allowfullscreen") + ? 'true' : 'false'; ?> getVar("gallery/caption"))) : ?> - "showCaption": getVar("gallery/caption") ? 'true' : 'false'; ?>, + ,"showCaption": getVar("gallery/caption") ? 'true' : 'false'; ?> getImageAttribute('product_page_image_medium', 'width'); $thumbWidth = $block->getImageAttribute('product_page_image_small', 'width'); - ?> - "width": "escapeHtml($imgWidth) ?>", - "thumbwidth": "escapeHtml($thumbWidth) ?>", - getImageAttribute('product_page_image_medium', 'height') + ?: $block->getImageAttribute('product_page_image_medium', 'width'); $thumbHeight = $block->getImageAttribute('product_page_image_small', 'height') ?: $block->getImageAttribute('product_page_image_small', 'width'); ?> - - "thumbheight": escapeHtml($thumbHeight); ?>, + ,"width": + escapeHtml($imgWidth) ?> + ,"thumbwidth": escapeHtml($thumbWidth) ?> + + ,"height": escapeHtml($imgHeight); ?> - getVar("gallery/thumbmargin"))) : ?> - "thumbmargin": getVar("gallery/thumbmargin"); ?>, + + ,"thumbheight": + escapeHtml($thumbHeight); ?> - getImageAttribute('product_page_image_medium', 'height') - ?: $block->getImageAttribute('product_page_image_medium', 'width') - ?> - - "height": escapeHtml($imgHeight); ?>, + getVar("gallery/thumbmargin")) : ?> + ,"thumbmargin": getVar("gallery/thumbmargin"); ?> getVar("gallery/transition/duration")) : ?> - "transitionduration": escapeHtml($block->getVar("gallery/transition/duration")) ?>, + ,"transitionduration": escapeHtml($block->getVar("gallery/transition/duration")) ?> + + getVar("gallery/transition/effect")) : ?> + ,"transition": "escapeHtml($block->getVar("gallery/transition/effect")) ?>" + + getVar("gallery/navarrows"))) : ?> + ,"navarrows": getVar("gallery/navarrows") ? 'true' : 'false'; ?> + + getVar("gallery/navtype")) : ?> + ,"navtype": "escapeHtml($block->getVar("gallery/navtype")) ?>" - "transition": "escapeHtml($block->getVar("gallery/transition/effect")) ?>", - getVar("gallery/navarrows"))) : ?> - "navarrows": escapeHtml($block->getVar("gallery/navarrows")) ?>, + getVar("gallery/navdir")) : ?> + ,"navdir": "escapeHtml($block->getVar("gallery/navdir")) ?>" - "navtype": "escapeHtml($block->getVar("gallery/navtype")) ?>", - "navdir": "escapeHtml($block->getVar("gallery/navdir")) ?>" }, "fullscreen": { - "nav": "escapeHtml($block->getVar("gallery/fullscreen/nav")) ?>", - getVar("gallery/fullscreen/loop")) : ?> - "loop": escapeHtml($block->getVar("gallery/fullscreen/loop")) ?>, + getVar("gallery/fullscreen/nav"))) : ?> + "nav": getVar("gallery/fullscreen/nav") ? '"true"' : '"false"'; ?> + + "nav": "escapeHtml($block->getVar("gallery/fullscreen/nav")) ?>" - "navdir": "escapeHtml($block->getVar("gallery/fullscreen/navdir")) ?>", - getVar("gallery/transition/navarrows")) : ?> - "navarrows": escapeHtml($block->getVar("gallery/fullscreen/navarrows")) ?>, + getVar("gallery/fullscreen/loop"))) : ?> + ,"loop": getVar("gallery/fullscreen/loop") ? 'true' : 'false'; ?> - "navtype": "escapeHtml($block->getVar("gallery/fullscreen/navtype")) ?>", - getVar("gallery/fullscreen/arrows")) : ?> - "arrows": escapeHtml($block->getVar("gallery/fullscreen/arrows")) ?>, + getVar("gallery/fullscreen/navtype")) : ?> + ,"navtype": "escapeHtml($block->getVar("gallery/fullscreen/navtype")) ?>" + + getVar("gallery/fullscreen/navdir")) : ?> + ,"navdir": "escapeHtml($block->getVar("gallery/fullscreen/navdir")) ?>" + + getVar("gallery/fullscreen/navarrows")) : ?> + ,"navarrows": "escapeHtml($block->getVar("gallery/fullscreen/navarrows")) ?>" + + getVar("gallery/fullscreen/arrows"))) : ?> + ,"arrows": getVar("gallery/fullscreen/arrows") + ? 'true' : 'false'; ?> getVar("gallery/fullscreen/caption"))) : ?> - getVar("gallery/fullscreen/caption") ? 'true' : 'false'; ?> - "showCaption": , + ,"showCaption": getVar("gallery/fullscreen/caption") + ? 'true' : 'false'; ?> getVar("gallery/fullscreen/transition/duration")) : ?> - "transitionduration": escapeHtml($block->getVar("gallery/fullscreen/transition/duration")) ?>, + ,"transitionduration": escapeHtml( + $block->getVar("gallery/fullscreen/transition/duration") + ); ?> + + getVar("gallery/fullscreen/transition/effect")) : ?> + ,"transition": "escapeHtml( + $block->getVar("gallery/fullscreen/transition/effect") + ); ?>" - "transition": "escapeHtml($block->getVar("gallery/fullscreen/transition/effect")) ?>" }, "breakpoints": getBreakpoints() ?> } From fc510dd82afd40ec51c1332f0404d42c2dfb217a Mon Sep 17 00:00:00 2001 From: gwharton Date: Tue, 4 Sep 2018 18:58:24 +0100 Subject: [PATCH 02/17] Fixed long lines --- .../view/frontend/templates/product/view/gallery.phtml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml b/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml index 13ac0c9937718..e8f60d46e0cd0 100644 --- a/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml +++ b/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml @@ -130,14 +130,12 @@ ? 'true' : 'false'; ?> getVar("gallery/fullscreen/transition/duration")) : ?> - ,"transitionduration": escapeHtml( - $block->getVar("gallery/fullscreen/transition/duration") - ); ?> + ,"transitionduration": + escapeHtml($block->getVar("gallery/fullscreen/transition/duration")); ?> getVar("gallery/fullscreen/transition/effect")) : ?> - ,"transition": "escapeHtml( - $block->getVar("gallery/fullscreen/transition/effect") - ); ?>" + ,"transition": + "escapeHtml($block->getVar("gallery/fullscreen/transition/effect")); ?>" }, "breakpoints": getBreakpoints() ?> From de19c3d59ef62f911766bfcc4ceb7ee74867994b Mon Sep 17 00:00:00 2001 From: gwharton Date: Wed, 5 Sep 2018 11:32:42 +0100 Subject: [PATCH 03/17] Modified getVar function to return null if the config variable is not found. This is required now following the changes made in #12285 to allow the returning of boolean variable types from view.xml --- lib/internal/Magento/Framework/Config/View.php | 6 +++--- .../Magento/Framework/View/Element/AbstractBlock.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/Magento/Framework/Config/View.php b/lib/internal/Magento/Framework/Config/View.php index 05863caeec2b6..1e419e0f65cda 100644 --- a/lib/internal/Magento/Framework/Config/View.php +++ b/lib/internal/Magento/Framework/Config/View.php @@ -79,13 +79,13 @@ public function getVars($module) * * @param string $module * @param string $var - * @return string|false|array + * @return string|bool|null|array */ public function getVarValue($module, $var) { $this->initData(); if (!isset($this->data['vars'][$module])) { - return false; + return null; } $value = $this->data['vars'][$module]; @@ -93,7 +93,7 @@ public function getVarValue($module, $var) if (is_array($value) && isset($value[$node])) { $value = $value[$node]; } else { - return false; + return null; } } diff --git a/lib/internal/Magento/Framework/View/Element/AbstractBlock.php b/lib/internal/Magento/Framework/View/Element/AbstractBlock.php index 84dd4270c59cf..c802ed5be1912 100644 --- a/lib/internal/Magento/Framework/View/Element/AbstractBlock.php +++ b/lib/internal/Magento/Framework/View/Element/AbstractBlock.php @@ -1142,7 +1142,7 @@ protected function _getSidPlaceholder($cacheKey = null) * * @param string $name variable name * @param string|null $module optional module name - * @return string|false + * @return string|bool|null|array */ public function getVar($name, $module = null) { From 62b711ec2cf7f3210005374a2d5762306413db05 Mon Sep 17 00:00:00 2001 From: gwharton Date: Wed, 5 Sep 2018 11:33:04 +0100 Subject: [PATCH 04/17] Rewrote both Gallery Options and Gallery Fullscreen Options output to be generated by block function instead of inline in the template. --- .../Catalog/Block/Product/View/Gallery.php | 140 ++++++++++++++++++ .../templates/product/view/gallery.phtml | 97 +----------- 2 files changed, 142 insertions(+), 95 deletions(-) diff --git a/app/code/Magento/Catalog/Block/Product/View/Gallery.php b/app/code/Magento/Catalog/Block/Product/View/Gallery.php index 17828b9d375d3..87b0a789452f3 100644 --- a/app/code/Magento/Catalog/Block/Product/View/Gallery.php +++ b/app/code/Magento/Catalog/Block/Product/View/Gallery.php @@ -138,6 +138,146 @@ public function getGalleryImagesJson() return json_encode($imagesItems); } + /** + * Retrieve gallery options in JSON format + * + * @return string + */ + public function getGalleryOptionsJson() + { + $optionItems = null; + + //Need to catch the special case that if gallery/nav is false, we need to output + //the string "false", and not the boolean false. Otherwise output string. + //True is not a valid option, but is left in incase someone sets it to true + //by accident. + if (is_bool($this->getVar("gallery/nav"))) { + $optionItems['nav'] = $this->getVar("gallery/nav") ? 'true' : 'false'; + } elseif ($this->getVar("gallery/nav") != null) { + $optionItems['nav'] = $this->getVar("gallery/nav"); + } + + if (is_bool($this->getVar("gallery/loop"))) { + $optionItems['loop'] = $this->getVar("gallery/loop"); + } + + if (is_bool($this->getVar("gallery/keyboard"))) { + $optionItems['keyboard'] = $this->getVar("gallery/keyboard"); + } + + if (is_bool($this->getVar("gallery/arrows"))) { + $optionItems['arrows'] = $this->getVar("gallery/arrows"); + } + + if (is_bool($this->getVar("gallery/allowfullscreen"))) { + $optionItems['allowfullscreen'] = $this->getVar("gallery/allowfullscreen"); + } + + if (is_bool($this->getVar("gallery/caption"))) { + $optionItems['showCaption'] = $this->getVar("gallery/caption"); + } + + $optionItems['width'] = (int)$this->escapeHtml( + $this->getImageAttribute('product_page_image_medium', 'width') + ); + + $optionItems['thumbwidth'] = (int)$this->escapeHtml( + $this->getImageAttribute('product_page_image_small', 'width') + ); + + $imgHeight = $this->getImageAttribute('product_page_image_medium', 'height') + ?: $this->getImageAttribute('product_page_image_medium', 'width'); + if ($imgHeight) { + $optionItems['height'] = (int)$this->escapeHtml($imgHeight); + } + + $thumbHeight = $this->getImageAttribute('product_page_image_small', 'height') + ?: $this->getImageAttribute('product_page_image_small', 'width'); + if ($thumbHeight) { + $optionItems['thumbheight'] = (int)$this->escapeHtml($thumbHeight); + } + + if ($this->getVar("gallery/thumbmargin")) { + $optionItems['thumbmargin'] = (int)$this->getVar("gallery/thumbmargin"); + } + + if ($this->getVar("gallery/transition/duration")) { + $optionItems['transitionduration'] = (int)$this->getVar("gallery/transition/duration"); + } + + if ($this->getVar("gallery/transition/effect")) { + $optionItems['transition'] = $this->getVar("gallery/transition/effect"); + } + + if (is_bool($this->getVar("gallery/navarrows"))) { + $optionItems['navarrows'] = $this->getVar("gallery/navarrows"); + } + + if ($this->getVar("gallery/navtype")) { + $optionItems['navtype'] = $this->getVar("gallery/navtype"); + } + + if ($this->getVar("gallery/navdir")) { + $optionItems['navdir'] = $this->getVar("gallery/navdir"); + } + + return json_encode($optionItems); + } + + /** + * Retrieve gallery fullscreen options in JSON format + * + * @return string + */ + public function getGalleryFSOptionsJson() + { + $fsOptionItems = null; + + //Need to catch the special case that if gallery/nav is false, we need to output + //the string "false", and not the boolean false. Otherwise output string. + //True is not a valid option, but is left in incase someone sets it to true + //by accident. + if (is_bool($this->getVar("gallery/fullscreen/nav"))) { + $fsOptionItems['nav'] = $this->getVar("gallery/fullscreen/nav") ? 'true' : 'false'; + } elseif ($this->getVar("gallery/fullscreen/nav") != null) { + $fsOptionItems['nav'] = $this->getVar("gallery/fullscreen/nav"); + } + + if (is_bool($this->getVar("gallery/fullscreen/loop"))) { + $fsOptionItems['loop'] = $this->getVar("gallery/fullscreen/loop"); + } + + if ($this->getVar("gallery/fullscreen/navtype")) { + $fsOptionItems['navtype'] = $this->getVar("gallery/fullscreen/navtype"); + } + + if ($this->getVar("gallery/fullscreen/navdir")) { + $fsOptionItems['navdir'] = $this->getVar("gallery/fullscreen/navdir"); + } + + if (is_bool($this->getVar("gallery/fullscreen/arrows"))) { + $fsOptionItems['arrows'] = $this->getVar("gallery/fullscreen/arrows"); + } + + if (is_bool($this->getVar("gallery/fullscreen/navarrows"))) { + $fsOptionItems['navarrows'] = $this->getVar("gallery/fullscreen/navarrows"); + } + + if (is_bool($this->getVar("gallery/fullscreen/caption"))) { + $fsOptionItems['showCaption'] = $this->getVar("gallery/fullscreen/caption"); + } + + if ($this->getVar("gallery/fullscreen/transition/duration")) { + $fsOptionItems['transitionduration'] = (int)$this->getVar("gallery/fullscreen/transition/duration"); + } + + if ($this->getVar("gallery/fullscreen/transition/effect")) { + $fsOptionItems['transition'] = $this->getVar("gallery/fullscreen/transition/effect"); + } + + return json_encode($fsOptionItems); + } + /** * Retrieve gallery url * diff --git a/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml b/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml index e8f60d46e0cd0..8b178d9981d97 100644 --- a/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml +++ b/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml @@ -43,101 +43,8 @@ "mixins":["magnifier/magnify"], "magnifierOpts": getMagnifier() ?>, "data": getGalleryImagesJson() ?>, - "options": { - getVar("gallery/nav"))) : ?> - "nav": getVar("gallery/nav") ? '"true"' : '"false"'; ?> - - "nav": "escapeHtml($block->getVar("gallery/nav")) ?>" - - getVar("gallery/loop"))) : ?> - ,"loop": getVar("gallery/loop") ? 'true' : 'false'; ?> - - getVar("gallery/keyboard"))) : ?> - ,"keyboard": - getVar("gallery/keyboard") ? 'true' : 'false'; ?> - - getVar("gallery/arrows"))) : ?> - ,"arrows": getVar("gallery/arrows") ? 'true' : 'false'; ?> - - getVar("gallery/allowfullscreen"))) : ?> - ,"allowfullscreen": getVar("gallery/allowfullscreen") - ? 'true' : 'false'; ?> - - getVar("gallery/caption"))) : ?> - ,"showCaption": getVar("gallery/caption") ? 'true' : 'false'; ?> - - getImageAttribute('product_page_image_medium', 'width'); - $thumbWidth = $block->getImageAttribute('product_page_image_small', 'width'); - $imgHeight = $block->getImageAttribute('product_page_image_medium', 'height') - ?: $block->getImageAttribute('product_page_image_medium', 'width'); - $thumbHeight = $block->getImageAttribute('product_page_image_small', 'height') - ?: $block->getImageAttribute('product_page_image_small', 'width'); - ?> - ,"width": - escapeHtml($imgWidth) ?> - ,"thumbwidth": escapeHtml($thumbWidth) ?> - - ,"height": escapeHtml($imgHeight); ?> - - - ,"thumbheight": - escapeHtml($thumbHeight); ?> - - getVar("gallery/thumbmargin")) : ?> - ,"thumbmargin": getVar("gallery/thumbmargin"); ?> - - getVar("gallery/transition/duration")) : ?> - ,"transitionduration": escapeHtml($block->getVar("gallery/transition/duration")) ?> - - getVar("gallery/transition/effect")) : ?> - ,"transition": "escapeHtml($block->getVar("gallery/transition/effect")) ?>" - - getVar("gallery/navarrows"))) : ?> - ,"navarrows": getVar("gallery/navarrows") ? 'true' : 'false'; ?> - - getVar("gallery/navtype")) : ?> - ,"navtype": "escapeHtml($block->getVar("gallery/navtype")) ?>" - - getVar("gallery/navdir")) : ?> - ,"navdir": "escapeHtml($block->getVar("gallery/navdir")) ?>" - - }, - "fullscreen": { - getVar("gallery/fullscreen/nav"))) : ?> - "nav": getVar("gallery/fullscreen/nav") ? '"true"' : '"false"'; ?> - - "nav": "escapeHtml($block->getVar("gallery/fullscreen/nav")) ?>" - - getVar("gallery/fullscreen/loop"))) : ?> - ,"loop": getVar("gallery/fullscreen/loop") ? 'true' : 'false'; ?> - - getVar("gallery/fullscreen/navtype")) : ?> - ,"navtype": "escapeHtml($block->getVar("gallery/fullscreen/navtype")) ?>" - - getVar("gallery/fullscreen/navdir")) : ?> - ,"navdir": "escapeHtml($block->getVar("gallery/fullscreen/navdir")) ?>" - - getVar("gallery/fullscreen/navarrows")) : ?> - ,"navarrows": "escapeHtml($block->getVar("gallery/fullscreen/navarrows")) ?>" - - getVar("gallery/fullscreen/arrows"))) : ?> - ,"arrows": getVar("gallery/fullscreen/arrows") - ? 'true' : 'false'; ?> - - getVar("gallery/fullscreen/caption"))) : ?> - ,"showCaption": getVar("gallery/fullscreen/caption") - ? 'true' : 'false'; ?> - - getVar("gallery/fullscreen/transition/duration")) : ?> - ,"transitionduration": - escapeHtml($block->getVar("gallery/fullscreen/transition/duration")); ?> - - getVar("gallery/fullscreen/transition/effect")) : ?> - ,"transition": - "escapeHtml($block->getVar("gallery/fullscreen/transition/effect")); ?>" - - }, + "options": getGalleryOptionsJson() ?>, + "fullscreen": getGalleryFSOptionsJson() ?>, "breakpoints": getBreakpoints() ?> } } From 17739f488ac600a42b511a8bb2ae9dbe6e7bd97a Mon Sep 17 00:00:00 2001 From: gwharton Date: Wed, 5 Sep 2018 12:43:53 +0100 Subject: [PATCH 05/17] Supress Cyclomatic Complexity and NPath Complexity checks --- app/code/Magento/Catalog/Block/Product/View/Gallery.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/code/Magento/Catalog/Block/Product/View/Gallery.php b/app/code/Magento/Catalog/Block/Product/View/Gallery.php index 87b0a789452f3..5862300e4f9f7 100644 --- a/app/code/Magento/Catalog/Block/Product/View/Gallery.php +++ b/app/code/Magento/Catalog/Block/Product/View/Gallery.php @@ -142,6 +142,8 @@ public function getGalleryImagesJson() * Retrieve gallery options in JSON format * * @return string + * @SuppressWarnings(PHPMD.CyclomaticComplexity) + * @SuppressWarnings(PHPMD.NPathComplexity) */ public function getGalleryOptionsJson() { @@ -228,6 +230,8 @@ public function getGalleryOptionsJson() * Retrieve gallery fullscreen options in JSON format * * @return string + * @SuppressWarnings(PHPMD.CyclomaticComplexity) + * @SuppressWarnings(PHPMD.NPathComplexity) */ public function getGalleryFSOptionsJson() { From eda62ccd997c05cbb3589563f54cd2d5d5caa711 Mon Sep 17 00:00:00 2001 From: gwharton Date: Wed, 5 Sep 2018 13:34:22 +0100 Subject: [PATCH 06/17] Removal of GalleryTest Integration test. This will be covered by full unit tests of all gallery variables. --- .../Block/Product/View/GalleryTest.php | 50 ------------------- 1 file changed, 50 deletions(-) delete mode 100644 dev/tests/integration/testsuite/Magento/Catalog/Block/Product/View/GalleryTest.php diff --git a/dev/tests/integration/testsuite/Magento/Catalog/Block/Product/View/GalleryTest.php b/dev/tests/integration/testsuite/Magento/Catalog/Block/Product/View/GalleryTest.php deleted file mode 100644 index c97dc821913f9..0000000000000 --- a/dev/tests/integration/testsuite/Magento/Catalog/Block/Product/View/GalleryTest.php +++ /dev/null @@ -1,50 +0,0 @@ -objectManager = Bootstrap::getObjectManager(); - } - /** - * Tests rendered gallery block. - * - * @magentoDataFixture Magento/Catalog/_files/product_with_image.php - * @magentoAppArea frontend - */ - public function testHtml() - { - /** @var ProductRepositoryInterface $productRepository */ - $productRepository = $this->objectManager->create(ProductRepositoryInterface::class); - /** @var ProductInterface $product */ - $product = $productRepository->get('simple'); - /** @var LayoutInterface $layout */ - $layout = $this->objectManager->get(LayoutInterface::class); - /** @var Gallery $block */ - $block = $layout->createBlock(Gallery::class); - $block->setData('product', $product); - $block->setTemplate("Magento_Catalog::product/view/gallery.phtml"); - - $showCaption = $block->getVar('gallery/caption'); - - self::assertContains('"showCaption": ' . $showCaption, $block->toHtml()); - } -} From 1033b04e1d6919614ee607ae3adf64136c63a752 Mon Sep 17 00:00:00 2001 From: gwharton Date: Wed, 5 Sep 2018 22:46:34 +0100 Subject: [PATCH 07/17] Removed detection of boolean config variables. All variables now returned as strings. This reverts changes made in #12285. --- .../testsuite/Magento/Framework/Config/ConverterTest.php | 5 +---- lib/internal/Magento/Framework/Config/Converter.php | 4 +--- lib/internal/Magento/Framework/Config/View.php | 6 +++--- .../Magento/Framework/View/Element/AbstractBlock.php | 2 +- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php b/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php index 1d2e090d1923b..9978100fad327 100644 --- a/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php +++ b/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php @@ -57,8 +57,6 @@ public function parseVarElementDataProvider() some string 1 0 - true - false XML; @@ -69,8 +67,7 @@ public function parseVarElementDataProvider() 'str' => 'some string', 'int-1' => '1', 'int-0' => '0', - 'bool-true' => true, - 'bool-false' => false + 'nonexistent' => false ] ] ]; diff --git a/lib/internal/Magento/Framework/Config/Converter.php b/lib/internal/Magento/Framework/Config/Converter.php index 3e66f641b8697..0401471f27ea5 100644 --- a/lib/internal/Magento/Framework/Config/Converter.php +++ b/lib/internal/Magento/Framework/Config/Converter.php @@ -103,9 +103,7 @@ protected function parseVarElement(\DOMElement $node) } } if (!count($result)) { - $result = (strtolower($node->nodeValue) !== 'true' && strtolower($node->nodeValue) !== 'false') - ? $node->nodeValue - : filter_var($node->nodeValue, FILTER_VALIDATE_BOOLEAN); + $result = $node->nodeValue; } return $result; } diff --git a/lib/internal/Magento/Framework/Config/View.php b/lib/internal/Magento/Framework/Config/View.php index 1e419e0f65cda..05863caeec2b6 100644 --- a/lib/internal/Magento/Framework/Config/View.php +++ b/lib/internal/Magento/Framework/Config/View.php @@ -79,13 +79,13 @@ public function getVars($module) * * @param string $module * @param string $var - * @return string|bool|null|array + * @return string|false|array */ public function getVarValue($module, $var) { $this->initData(); if (!isset($this->data['vars'][$module])) { - return null; + return false; } $value = $this->data['vars'][$module]; @@ -93,7 +93,7 @@ public function getVarValue($module, $var) if (is_array($value) && isset($value[$node])) { $value = $value[$node]; } else { - return null; + return false; } } diff --git a/lib/internal/Magento/Framework/View/Element/AbstractBlock.php b/lib/internal/Magento/Framework/View/Element/AbstractBlock.php index c802ed5be1912..5c74edc9d4c6c 100644 --- a/lib/internal/Magento/Framework/View/Element/AbstractBlock.php +++ b/lib/internal/Magento/Framework/View/Element/AbstractBlock.php @@ -1142,7 +1142,7 @@ protected function _getSidPlaceholder($cacheKey = null) * * @param string $name variable name * @param string|null $module optional module name - * @return string|bool|null|array + * @return string|false|array */ public function getVar($name, $module = null) { From 7985068302fb02b2059336b0a0939dd82c634841 Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 00:12:27 +0100 Subject: [PATCH 08/17] Updated getBreakpoints to convert string booleans to actual booleans. Updated Options and Fullscreen Options functions. --- .../Catalog/Block/Product/View/Gallery.php | 188 +++++++++--------- 1 file changed, 95 insertions(+), 93 deletions(-) diff --git a/app/code/Magento/Catalog/Block/Product/View/Gallery.php b/app/code/Magento/Catalog/Block/Product/View/Gallery.php index 5862300e4f9f7..be63c9b4451df 100644 --- a/app/code/Magento/Catalog/Block/Product/View/Gallery.php +++ b/app/code/Magento/Catalog/Block/Product/View/Gallery.php @@ -100,7 +100,9 @@ public function getMagnifier() */ public function getBreakpoints() { - return $this->jsonEncoder->encode($this->getVar('breakpoints')); + $breakpoints = $this->getVar('breakpoints'); + array_walk_recursive($breakpoints, [$this, 'convertBooleans']); + return $this->jsonEncoder->encode($breakpoints); } /** @@ -138,6 +140,23 @@ public function getGalleryImagesJson() return json_encode($imagesItems); } + /** + * Convert string to booleans + * + * @param string $item + * @param string $key + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + private function convertBooleans(&$item, $key) + { + if (strtolower($item) == 'true') { + $item = true; + } + if (strtolower($item) == 'false') { + $item = false; + } + } + /** * Retrieve gallery options in JSON format * @@ -148,81 +167,69 @@ public function getGalleryImagesJson() public function getGalleryOptionsJson() { $optionItems = null; - - //Need to catch the special case that if gallery/nav is false, we need to output - //the string "false", and not the boolean false. Otherwise output string. - //True is not a valid option, but is left in incase someone sets it to true - //by accident. - if (is_bool($this->getVar("gallery/nav"))) { - $optionItems['nav'] = $this->getVar("gallery/nav") ? 'true' : 'false'; - } elseif ($this->getVar("gallery/nav") != null) { - $optionItems['nav'] = $this->getVar("gallery/nav"); + + if ($this->getVar("gallery/nav")) { + $optionItems['nav'] = $this->escapeHtml($this->getVar("gallery/nav")); } - - if (is_bool($this->getVar("gallery/loop"))) { - $optionItems['loop'] = $this->getVar("gallery/loop"); + if ($this->getVar("gallery/loop")) { + $optionItems['loop'] = filter_var($this->getVar("gallery/loop"), FILTER_VALIDATE_BOOLEAN); } - - if (is_bool($this->getVar("gallery/keyboard"))) { - $optionItems['keyboard'] = $this->getVar("gallery/keyboard"); + if ($this->getVar("gallery/keyboard")) { + $optionItems['keyboard'] = filter_var($this->getVar("gallery/keyboard"), FILTER_VALIDATE_BOOLEAN); } - - if (is_bool($this->getVar("gallery/arrows"))) { - $optionItems['arrows'] = $this->getVar("gallery/arrows"); + if ($this->getVar("gallery/arrows")) { + $optionItems['arrows'] = filter_var($this->getVar("gallery/arrows"), FILTER_VALIDATE_BOOLEAN); } - - if (is_bool($this->getVar("gallery/allowfullscreen"))) { - $optionItems['allowfullscreen'] = $this->getVar("gallery/allowfullscreen"); + if ($this->getVar("gallery/caption")) { + $optionItems['showCaption'] = filter_var($this->getVar("gallery/caption"), FILTER_VALIDATE_BOOLEAN); } - - if (is_bool($this->getVar("gallery/caption"))) { - $optionItems['showCaption'] = $this->getVar("gallery/caption"); + if ($this->getVar("gallery/allowfullscreen")) { + $optionItems['allowfullscreen'] = filter_var( + $this->getVar("gallery/allowfullscreen"), + FILTER_VALIDATE_BOOLEAN + ); + } + if ($this->getVar("gallery/navdir")) { + $optionItems['navdir'] = $this->escapeHtml($this->getVar("gallery/navdir")); } - - $optionItems['width'] = (int)$this->escapeHtml( - $this->getImageAttribute('product_page_image_medium', 'width') + if ($this->getVar("gallery/navarrows")) { + $optionItems['navarrows'] = filter_var($this->getVar("gallery/navarrows"), FILTER_VALIDATE_BOOLEAN); + } + if ($this->getVar("gallery/navtype")) { + $optionItems['navtype'] = $this->escapeHtml($this->getVar("gallery/navtype")); + } + if ($this->getVar("gallery/thumbmargin")) { + $optionItems['thumbmargin'] = filter_var($this->getVar("gallery/thumbmargin"), FILTER_VALIDATE_INT); + } + if ($this->getVar("gallery/transition/effect")) { + $optionItems['transition'] = $this->escapeHtml($this->getVar("gallery/transition/effect")); + } + if ($this->getVar("gallery/transition/duration")) { + $optionItems['transitionduration'] = filter_var( + $this->getVar("gallery/transition/duration"), + FILTER_VALIDATE_INT + ); + } + + $optionItems['width'] = filter_var( + $this->getImageAttribute('product_page_image_medium', 'width'), + FILTER_VALIDATE_INT ); - - $optionItems['thumbwidth'] = (int)$this->escapeHtml( - $this->getImageAttribute('product_page_image_small', 'width') + $optionItems['thumbwidth'] = filter_var( + $this->getImageAttribute('product_page_image_small', 'width'), + FILTER_VALIDATE_INT ); - $imgHeight = $this->getImageAttribute('product_page_image_medium', 'height') ?: $this->getImageAttribute('product_page_image_medium', 'width'); if ($imgHeight) { - $optionItems['height'] = (int)$this->escapeHtml($imgHeight); + $optionItems['height'] = filter_var($imgHeight, FILTER_VALIDATE_INT); } - $thumbHeight = $this->getImageAttribute('product_page_image_small', 'height') ?: $this->getImageAttribute('product_page_image_small', 'width'); if ($thumbHeight) { - $optionItems['thumbheight'] = (int)$this->escapeHtml($thumbHeight); + $optionItems['thumbheight'] = filter_var($thumbHeight, FILTER_VALIDATE_INT); } - - if ($this->getVar("gallery/thumbmargin")) { - $optionItems['thumbmargin'] = (int)$this->getVar("gallery/thumbmargin"); - } - - if ($this->getVar("gallery/transition/duration")) { - $optionItems['transitionduration'] = (int)$this->getVar("gallery/transition/duration"); - } - - if ($this->getVar("gallery/transition/effect")) { - $optionItems['transition'] = $this->getVar("gallery/transition/effect"); - } - - if (is_bool($this->getVar("gallery/navarrows"))) { - $optionItems['navarrows'] = $this->getVar("gallery/navarrows"); - } - - if ($this->getVar("gallery/navtype")) { - $optionItems['navtype'] = $this->getVar("gallery/navtype"); - } - - if ($this->getVar("gallery/navdir")) { - $optionItems['navdir'] = $this->getVar("gallery/navdir"); - } - + return json_encode($optionItems); } @@ -236,49 +243,44 @@ public function getGalleryOptionsJson() public function getGalleryFSOptionsJson() { $fsOptionItems = null; - - //Need to catch the special case that if gallery/nav is false, we need to output - //the string "false", and not the boolean false. Otherwise output string. - //True is not a valid option, but is left in incase someone sets it to true - //by accident. - if (is_bool($this->getVar("gallery/fullscreen/nav"))) { - $fsOptionItems['nav'] = $this->getVar("gallery/fullscreen/nav") ? 'true' : 'false'; - } elseif ($this->getVar("gallery/fullscreen/nav") != null) { - $fsOptionItems['nav'] = $this->getVar("gallery/fullscreen/nav"); - } - if (is_bool($this->getVar("gallery/fullscreen/loop"))) { - $fsOptionItems['loop'] = $this->getVar("gallery/fullscreen/loop"); + if ($this->getVar("gallery/fullscreen/nav")) { + $fsOptionItems['nav'] = $this->escapeHtml($this->getVar("gallery/fullscreen/nav")); } - - if ($this->getVar("gallery/fullscreen/navtype")) { - $fsOptionItems['navtype'] = $this->getVar("gallery/fullscreen/navtype"); + if ($this->getVar("gallery/fullscreen/loop")) { + $fsOptionItems['loop'] = filter_var($this->getVar("gallery/fullscreen/loop"), FILTER_VALIDATE_BOOLEAN); } - - if ($this->getVar("gallery/fullscreen/navdir")) { - $fsOptionItems['navdir'] = $this->getVar("gallery/fullscreen/navdir"); + if ($this->getVar("gallery/fullscreen/arrows")) { + $fsOptionItems['arrows'] = filter_var($this->getVar("gallery/fullscreen/arrows"), FILTER_VALIDATE_BOOLEAN); } - - if (is_bool($this->getVar("gallery/fullscreen/arrows"))) { - $fsOptionItems['arrows'] = $this->getVar("gallery/fullscreen/arrows"); + if ($this->getVar("gallery/fullscreen/caption")) { + $fsOptionItems['showCaption'] = filter_var( + $this->getVar("gallery/fullscreen/caption"), + FILTER_VALIDATE_BOOLEAN + ); } - - if (is_bool($this->getVar("gallery/fullscreen/navarrows"))) { - $fsOptionItems['navarrows'] = $this->getVar("gallery/fullscreen/navarrows"); + if ($this->getVar("gallery/fullscreen/navdir")) { + $fsOptionItems['navdir'] = $this->escapeHtml($this->getVar("gallery/fullscreen/navdir")); } - - if (is_bool($this->getVar("gallery/fullscreen/caption"))) { - $fsOptionItems['showCaption'] = $this->getVar("gallery/fullscreen/caption"); + if ($this->getVar("gallery/fullscreen/navarrows")) { + $fsOptionItems['navarrows'] = filter_var( + $this->getVar("gallery/fullscreen/navarrows"), + FILTER_VALIDATE_BOOLEAN + ); } - - if ($this->getVar("gallery/fullscreen/transition/duration")) { - $fsOptionItems['transitionduration'] = (int)$this->getVar("gallery/fullscreen/transition/duration"); + if ($this->getVar("gallery/fullscreen/navtype")) { + $fsOptionItems['navtype'] = $this->escapeHtml($this->getVar("gallery/fullscreen/navtype")); } - if ($this->getVar("gallery/fullscreen/transition/effect")) { - $fsOptionItems['transition'] = $this->getVar("gallery/fullscreen/transition/effect"); + $fsOptionItems['transition'] = $this->escapeHtml($this->getVar("gallery/fullscreen/transition/effect")); } - + if ($this->getVar("gallery/fullscreen/transition/duration")) { + $fsOptionItems['transitionduration'] = filter_var( + $this->getVar("gallery/fullscreen/transition/duration"), + FILTER_VALIDATE_INT + ); + } + return json_encode($fsOptionItems); } From 65f1103e5f0781565a26b918d3e1cd9a0280d3cd Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 01:09:11 +0100 Subject: [PATCH 09/17] Changed type of convertBooleans callback to protected static --- app/code/Magento/Catalog/Block/Product/View/Gallery.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/code/Magento/Catalog/Block/Product/View/Gallery.php b/app/code/Magento/Catalog/Block/Product/View/Gallery.php index be63c9b4451df..65735921f06cc 100644 --- a/app/code/Magento/Catalog/Block/Product/View/Gallery.php +++ b/app/code/Magento/Catalog/Block/Product/View/Gallery.php @@ -147,7 +147,7 @@ public function getGalleryImagesJson() * @param string $key * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ - private function convertBooleans(&$item, $key) + protected static function convertBooleans(&$item, $key) { if (strtolower($item) == 'true') { $item = true; From 6d8d079616c703e5bce667057e23640c217cb8e2 Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 02:24:16 +0100 Subject: [PATCH 10/17] Removed ConverterTest integration test as no longer required. Converter does not convert any more. --- .../Framework/Config/ConverterTest.php | 91 ------------------- 1 file changed, 91 deletions(-) delete mode 100644 dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php diff --git a/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php b/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php deleted file mode 100644 index 9978100fad327..0000000000000 --- a/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php +++ /dev/null @@ -1,91 +0,0 @@ -loadXML($sourceString); - $actual = $this->converter->convert($document); - - self::assertEquals( - $expected, - $actual - ); - } - - /** - * Data provider for testParseVarElement. - * - * @return array - */ - public function parseVarElementDataProvider() - { - // @codingStandardsIgnoreStart - $sourceString = <<<'XML' - - - - some string - 1 - 0 - - -XML; - // @codingStandardsIgnoreEnd - $expectedResult = [ - 'vars' => [ - 'Magento_Test' => [ - 'str' => 'some string', - 'int-1' => '1', - 'int-0' => '0', - 'nonexistent' => false - ] - ] - ]; - - return [ - [ - $sourceString, - $expectedResult - ], - ]; - } - - /** - * @inheritdoc - */ - protected function setUp() - { - $this->objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager(); - $this->converter = $this->objectManager->get(Converter::class); - } -} From dada20c5065e0d97e68919549aac6ba3e033ae46 Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 13:11:15 +0100 Subject: [PATCH 11/17] Added unit tests --- .../Unit/Block/Product/View/GalleryTest.php | 141 ++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php index ae5176e78df7b..0970c321c3117 100644 --- a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php @@ -37,6 +37,26 @@ class GalleryTest extends \PHPUnit\Framework\TestCase */ protected $jsonEncoderMock; + /** + * @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager + */ + protected $objectManager; + + /** + * @var \Magento\Framework\Config\View + */ + protected $configView; + + /** + * @var \Magento\Framework\View\Config + */ + protected $viewConfig; + + /** + * @var \Magento\Framework\Escaper + */ + protected $escaper; + protected function setUp() { $this->mockContext(); @@ -75,6 +95,26 @@ protected function mockContext() $this->context->expects($this->any()) ->method('getRegistry') ->willReturn($this->registry); + + $this->objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this); + + $this->escaper = $this->objectManager->getObject(\Magento\Framework\Escaper::class); + $this->context->expects($this->any()) + ->method('getEscaper') + ->willReturn($this->escaper); + $this->viewConfig = $this->getMockBuilder(\Magento\Framework\View\Config::class) + ->disableOriginalConstructor() + ->getMock(); + $this->configView = $this->getMockBuilder(\Magento\Framework\Config\View::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->viewConfig->expects($this->any()) + ->method('getViewConfig') + ->willReturn($this->configView); + $this->context->expects($this->any()) + ->method('getViewConfig') + ->willReturn($this->viewConfig); } public function testGetGalleryImagesJsonWithLabel() @@ -262,4 +302,105 @@ private function getImagesCollectionWithPopulatedDataObject($hasLabel) return $collectionMock; } + + public function testGalleryOptions() + { + $configMap = [ + ['Magento_Catalog', 'gallery/nav', 'thumbs'], + ['Magento_Catalog', 'gallery/loop', 'false'], + ['Magento_Catalog', 'gallery/keyboard', 'true'], + ['Magento_Catalog', 'gallery/arrows', 'true'], + ['Magento_Catalog', 'gallery/caption', 'false'], + ['Magento_Catalog', 'gallery/allowfullscreen', 'true'], + ['Magento_Catalog', 'gallery/navdir', 'horizontal'], + ['Magento_Catalog', 'gallery/navarrows', 'true'], + ['Magento_Catalog', 'gallery/navtype', 'slides'], + ['Magento_Catalog', 'gallery/thumbmargin', '5'], + ['Magento_Catalog', 'gallery/transition/effect', 'slide'], + ['Magento_Catalog', 'gallery/transition/duration', '500'], + ]; + + $mediaAttributesMap = [ + [ + 'Magento_Catalog', + \Magento\Catalog\Helper\Image::MEDIA_TYPE_CONFIG_NODE, + 'product_page_image_medium', + [ + 'height' => 100, + 'width' => 200 + ] + ], + [ + 'Magento_Catalog', + \Magento\Catalog\Helper\Image::MEDIA_TYPE_CONFIG_NODE, + 'product_page_image_small', + [ + 'height' => 300, + 'width' => 400 + ] + ], + ]; + + $this->configView->expects($this->any()) + ->method('getVarValue') + ->will($this->returnValueMap($configMap)); + $this->configView->expects($this->any()) + ->method('getMediaAttributes') + ->will($this->returnValueMap($mediaAttributesMap)); + + $json = $this->model->getGalleryOptionsJson(); + $decodedJson = json_decode($json, true); + + $this->assertEquals('thumbs', $decodedJson['nav']); + $this->assertEquals(false, $decodedJson['loop']); + $this->assertEquals(true, $decodedJson['keyboard']); + $this->assertEquals(true, $decodedJson['arrows']); + $this->assertEquals(false, $decodedJson['showCaption']); + $this->assertEquals(true, $decodedJson['allowfullscreen']); + $this->assertEquals('horizontal', $decodedJson['navdir']); + $this->assertEquals(true, $decodedJson['navarrows']); + $this->assertEquals('slides', $decodedJson['navtype']); + $this->assertEquals(5, $decodedJson['thumbmargin']); + $this->assertEquals('slide', $decodedJson['transition']); + $this->assertEquals(500, $decodedJson['transitionduration']); + $this->assertEquals(100, $decodedJson['height']); + $this->assertEquals(200, $decodedJson['width']); + $this->assertEquals(300, $decodedJson['thumbheight']); + $this->assertEquals(400, $decodedJson['thumbwidth']); + } + + public function testGalleryFSOptions() + { + $configMap = [ + ['Magento_Catalog', 'gallery/fullscreen/nav', 'false'], + ['Magento_Catalog', 'gallery/fullscreen/loop', 'true'], + ['Magento_Catalog', 'gallery/fullscreen/arrows', 'false'], + ['Magento_Catalog', 'gallery/fullscreen/caption', 'true'], + ['Magento_Catalog', 'gallery/fullscreen/navdir', 'vertical'], + ['Magento_Catalog', 'gallery/fullscreen/navarrows', 'false'], + ['Magento_Catalog', 'gallery/fullscreen/navtype', 'thumbs'], + ['Magento_Catalog', 'gallery/fullscreen/transition/effect', 'dissolve'], + ['Magento_Catalog', 'gallery/fullscreen/transition/duration', '300'] + ]; + + $this->configView->expects($this->any()) + ->method('getVarValue') + ->will($this->returnValueMap($configMap)); + + $json = $this->model->getGalleryFSOptionsJson(); + $decodedJson = json_decode($json, true); + + //Note, this tests the special case for nav variable set to false. It + //Should not be converted to boolean. + $this->assertEquals('false', $decodedJson['nav']); + + $this->assertEquals(true, $decodedJson['loop']); + $this->assertEquals(false, $decodedJson['arrows']); + $this->assertEquals(true, $decodedJson['showCaption']); + $this->assertEquals('vertical', $decodedJson['navdir']); + $this->assertEquals(false, $decodedJson['navarrows']); + $this->assertEquals('thumbs', $decodedJson['navtype']); + $this->assertEquals('dissolve', $decodedJson['transition']); + $this->assertEquals(300, $decodedJson['transitionduration']); + } } From 66b85e7eeba05102cc3aa27e730037ed7702fdbd Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 13:34:33 +0100 Subject: [PATCH 12/17] Added missing keyboard and thumbmargin options to fullscreen options --- app/code/Magento/Catalog/Block/Product/View/Gallery.php | 6 ++++++ .../Catalog/Test/Unit/Block/Product/View/GalleryTest.php | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/app/code/Magento/Catalog/Block/Product/View/Gallery.php b/app/code/Magento/Catalog/Block/Product/View/Gallery.php index 65735921f06cc..c2241b82cac13 100644 --- a/app/code/Magento/Catalog/Block/Product/View/Gallery.php +++ b/app/code/Magento/Catalog/Block/Product/View/Gallery.php @@ -250,6 +250,9 @@ public function getGalleryFSOptionsJson() if ($this->getVar("gallery/fullscreen/loop")) { $fsOptionItems['loop'] = filter_var($this->getVar("gallery/fullscreen/loop"), FILTER_VALIDATE_BOOLEAN); } + if ($this->getVar("gallery/fullscreen/keyboard")) { + $fsOptionItems['keyboard'] = filter_var($this->getVar("gallery/fullscreen/keyboard"), FILTER_VALIDATE_BOOLEAN); + } if ($this->getVar("gallery/fullscreen/arrows")) { $fsOptionItems['arrows'] = filter_var($this->getVar("gallery/fullscreen/arrows"), FILTER_VALIDATE_BOOLEAN); } @@ -271,6 +274,9 @@ public function getGalleryFSOptionsJson() if ($this->getVar("gallery/fullscreen/navtype")) { $fsOptionItems['navtype'] = $this->escapeHtml($this->getVar("gallery/fullscreen/navtype")); } + if ($this->getVar("gallery/fullscreen/thumbmargin")) { + $fsOptionItems['thumbmargin'] = filter_var($this->getVar("gallery/fullscreen/thumbmargin"), FILTER_VALIDATE_INT); + } if ($this->getVar("gallery/fullscreen/transition/effect")) { $fsOptionItems['transition'] = $this->escapeHtml($this->getVar("gallery/fullscreen/transition/effect")); } diff --git a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php index 0970c321c3117..d2c690449a37f 100644 --- a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php @@ -374,11 +374,13 @@ public function testGalleryFSOptions() $configMap = [ ['Magento_Catalog', 'gallery/fullscreen/nav', 'false'], ['Magento_Catalog', 'gallery/fullscreen/loop', 'true'], + ['Magento_Catalog', 'gallery/fullscreen/keyboard', 'false'], ['Magento_Catalog', 'gallery/fullscreen/arrows', 'false'], ['Magento_Catalog', 'gallery/fullscreen/caption', 'true'], ['Magento_Catalog', 'gallery/fullscreen/navdir', 'vertical'], ['Magento_Catalog', 'gallery/fullscreen/navarrows', 'false'], ['Magento_Catalog', 'gallery/fullscreen/navtype', 'thumbs'], + ['Magento_Catalog', 'gallery/fullscreen/thumbmargin', '10'], ['Magento_Catalog', 'gallery/fullscreen/transition/effect', 'dissolve'], ['Magento_Catalog', 'gallery/fullscreen/transition/duration', '300'] ]; @@ -396,9 +398,11 @@ public function testGalleryFSOptions() $this->assertEquals(true, $decodedJson['loop']); $this->assertEquals(false, $decodedJson['arrows']); + $this->assertEquals(false, $decodedJson['keyboard']); $this->assertEquals(true, $decodedJson['showCaption']); $this->assertEquals('vertical', $decodedJson['navdir']); $this->assertEquals(false, $decodedJson['navarrows']); + $this->assertEquals(10, $decodedJson['thumbmargin']); $this->assertEquals('thumbs', $decodedJson['navtype']); $this->assertEquals('dissolve', $decodedJson['transition']); $this->assertEquals(300, $decodedJson['transitionduration']); From c662eb2b85199464be97b261e6ad599c3fae2c71 Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 14:52:40 +0100 Subject: [PATCH 13/17] Fixed whitespace and line length violations --- .../Magento/Catalog/Block/Product/View/Gallery.php | 10 ++++++++-- .../Test/Unit/Block/Product/View/GalleryTest.php | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/code/Magento/Catalog/Block/Product/View/Gallery.php b/app/code/Magento/Catalog/Block/Product/View/Gallery.php index c2241b82cac13..3930b9ceb95c9 100644 --- a/app/code/Magento/Catalog/Block/Product/View/Gallery.php +++ b/app/code/Magento/Catalog/Block/Product/View/Gallery.php @@ -251,7 +251,10 @@ public function getGalleryFSOptionsJson() $fsOptionItems['loop'] = filter_var($this->getVar("gallery/fullscreen/loop"), FILTER_VALIDATE_BOOLEAN); } if ($this->getVar("gallery/fullscreen/keyboard")) { - $fsOptionItems['keyboard'] = filter_var($this->getVar("gallery/fullscreen/keyboard"), FILTER_VALIDATE_BOOLEAN); + $fsOptionItems['keyboard'] = filter_var( + $this->getVar("gallery/fullscreen/keyboard"), + FILTER_VALIDATE_BOOLEAN + ); } if ($this->getVar("gallery/fullscreen/arrows")) { $fsOptionItems['arrows'] = filter_var($this->getVar("gallery/fullscreen/arrows"), FILTER_VALIDATE_BOOLEAN); @@ -275,7 +278,10 @@ public function getGalleryFSOptionsJson() $fsOptionItems['navtype'] = $this->escapeHtml($this->getVar("gallery/fullscreen/navtype")); } if ($this->getVar("gallery/fullscreen/thumbmargin")) { - $fsOptionItems['thumbmargin'] = filter_var($this->getVar("gallery/fullscreen/thumbmargin"), FILTER_VALIDATE_INT); + $fsOptionItems['thumbmargin'] = filter_var( + $this->getVar("gallery/fullscreen/thumbmargin"), + FILTER_VALIDATE_INT + ); } if ($this->getVar("gallery/fullscreen/transition/effect")) { $fsOptionItems['transition'] = $this->escapeHtml($this->getVar("gallery/fullscreen/transition/effect")); diff --git a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php index d2c690449a37f..0773a5120e02a 100644 --- a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php @@ -360,7 +360,7 @@ public function testGalleryOptions() $this->assertEquals('horizontal', $decodedJson['navdir']); $this->assertEquals(true, $decodedJson['navarrows']); $this->assertEquals('slides', $decodedJson['navtype']); - $this->assertEquals(5, $decodedJson['thumbmargin']); + $this->assertEquals(5, $decodedJson['thumbmargin']); $this->assertEquals('slide', $decodedJson['transition']); $this->assertEquals(500, $decodedJson['transitionduration']); $this->assertEquals(100, $decodedJson['height']); @@ -406,5 +406,5 @@ public function testGalleryFSOptions() $this->assertEquals('thumbs', $decodedJson['navtype']); $this->assertEquals('dissolve', $decodedJson['transition']); $this->assertEquals(300, $decodedJson['transitionduration']); - } + } } From d8c92386cb271951ff1fba17d4dd9f2de39d4d20 Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 16:09:13 +0100 Subject: [PATCH 14/17] Suppress CouplingBetweenObjects warning on unit test --- .../Catalog/Test/Unit/Block/Product/View/GalleryTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php index 0773a5120e02a..9f0b0502f3333 100644 --- a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php @@ -5,6 +5,9 @@ */ namespace Magento\Catalog\Test\Unit\Block\Product\View; +/** + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + */ class GalleryTest extends \PHPUnit\Framework\TestCase { /** From 8fb042440b7324cd86eda804855adbd521da6839 Mon Sep 17 00:00:00 2001 From: gwharton Date: Thu, 6 Sep 2018 19:32:37 +0100 Subject: [PATCH 15/17] Fixed whitespace violations --- .../Test/Unit/Block/Product/View/GalleryTest.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php index 9f0b0502f3333..138b435216dc8 100644 --- a/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php @@ -98,7 +98,7 @@ protected function mockContext() $this->context->expects($this->any()) ->method('getRegistry') ->willReturn($this->registry); - + $this->objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this); $this->escaper = $this->objectManager->getObject(\Magento\Framework\Escaper::class); @@ -111,7 +111,7 @@ protected function mockContext() $this->configView = $this->getMockBuilder(\Magento\Framework\Config\View::class) ->disableOriginalConstructor() ->getMock(); - + $this->viewConfig->expects($this->any()) ->method('getViewConfig') ->willReturn($this->configView); @@ -305,7 +305,7 @@ private function getImagesCollectionWithPopulatedDataObject($hasLabel) return $collectionMock; } - + public function testGalleryOptions() { $configMap = [ @@ -322,7 +322,7 @@ public function testGalleryOptions() ['Magento_Catalog', 'gallery/transition/effect', 'slide'], ['Magento_Catalog', 'gallery/transition/duration', '500'], ]; - + $mediaAttributesMap = [ [ 'Magento_Catalog', @@ -343,7 +343,7 @@ public function testGalleryOptions() ] ], ]; - + $this->configView->expects($this->any()) ->method('getVarValue') ->will($this->returnValueMap($configMap)); @@ -371,7 +371,7 @@ public function testGalleryOptions() $this->assertEquals(300, $decodedJson['thumbheight']); $this->assertEquals(400, $decodedJson['thumbwidth']); } - + public function testGalleryFSOptions() { $configMap = [ @@ -387,7 +387,7 @@ public function testGalleryFSOptions() ['Magento_Catalog', 'gallery/fullscreen/transition/effect', 'dissolve'], ['Magento_Catalog', 'gallery/fullscreen/transition/duration', '300'] ]; - + $this->configView->expects($this->any()) ->method('getVarValue') ->will($this->returnValueMap($configMap)); @@ -398,7 +398,7 @@ public function testGalleryFSOptions() //Note, this tests the special case for nav variable set to false. It //Should not be converted to boolean. $this->assertEquals('false', $decodedJson['nav']); - + $this->assertEquals(true, $decodedJson['loop']); $this->assertEquals(false, $decodedJson['arrows']); $this->assertEquals(false, $decodedJson['keyboard']); From 053f53975b7145e58c6f84c03913649ef4f9d28c Mon Sep 17 00:00:00 2001 From: gwharton Date: Fri, 7 Sep 2018 14:28:09 +0100 Subject: [PATCH 16/17] Removed the changes that were split off into PR #17969 --- .../Catalog/Block/Product/View/Gallery.php | 21 +---- .../Framework/Config/ConverterTest.php | 94 +++++++++++++++++++ .../Magento/Framework/Config/Converter.php | 4 +- .../Framework/View/Element/AbstractBlock.php | 2 +- 4 files changed, 99 insertions(+), 22 deletions(-) create mode 100644 dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php diff --git a/app/code/Magento/Catalog/Block/Product/View/Gallery.php b/app/code/Magento/Catalog/Block/Product/View/Gallery.php index 3930b9ceb95c9..682250d42f81e 100644 --- a/app/code/Magento/Catalog/Block/Product/View/Gallery.php +++ b/app/code/Magento/Catalog/Block/Product/View/Gallery.php @@ -100,9 +100,7 @@ public function getMagnifier() */ public function getBreakpoints() { - $breakpoints = $this->getVar('breakpoints'); - array_walk_recursive($breakpoints, [$this, 'convertBooleans']); - return $this->jsonEncoder->encode($breakpoints); + return $this->jsonEncoder->encode($this->getVar('breakpoints')); } /** @@ -140,23 +138,6 @@ public function getGalleryImagesJson() return json_encode($imagesItems); } - /** - * Convert string to booleans - * - * @param string $item - * @param string $key - * @SuppressWarnings(PHPMD.UnusedFormalParameter) - */ - protected static function convertBooleans(&$item, $key) - { - if (strtolower($item) == 'true') { - $item = true; - } - if (strtolower($item) == 'false') { - $item = false; - } - } - /** * Retrieve gallery options in JSON format * diff --git a/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php b/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php new file mode 100644 index 0000000000000..767ab32f5d727 --- /dev/null +++ b/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php @@ -0,0 +1,94 @@ +loadXML($sourceString); + $actual = $this->converter->convert($document); + + self::assertEquals( + $expected, + $actual + ); + } + + /** + * Data provider for testParseVarElement. + * + * @return array + */ + public function parseVarElementDataProvider() + { + // @codingStandardsIgnoreStart + $sourceString = <<<'XML' + + + + some string + 1 + 0 + true + false + + +XML; + // @codingStandardsIgnoreEnd + $expectedResult = [ + 'vars' => [ + 'Magento_Test' => [ + 'str' => 'some string', + 'int-1' => '1', + 'int-0' => '0', + 'bool-true' => true, + 'bool-false' => false + ] + ] + ]; + + return [ + [ + $sourceString, + $expectedResult + ], + ]; + } + + /** + * @inheritdoc + */ + protected function setUp() + { + $this->objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager(); + $this->converter = $this->objectManager->get(Converter::class); + } +} \ No newline at end of file diff --git a/lib/internal/Magento/Framework/Config/Converter.php b/lib/internal/Magento/Framework/Config/Converter.php index 0401471f27ea5..3e66f641b8697 100644 --- a/lib/internal/Magento/Framework/Config/Converter.php +++ b/lib/internal/Magento/Framework/Config/Converter.php @@ -103,7 +103,9 @@ protected function parseVarElement(\DOMElement $node) } } if (!count($result)) { - $result = $node->nodeValue; + $result = (strtolower($node->nodeValue) !== 'true' && strtolower($node->nodeValue) !== 'false') + ? $node->nodeValue + : filter_var($node->nodeValue, FILTER_VALIDATE_BOOLEAN); } return $result; } diff --git a/lib/internal/Magento/Framework/View/Element/AbstractBlock.php b/lib/internal/Magento/Framework/View/Element/AbstractBlock.php index 5c74edc9d4c6c..84dd4270c59cf 100644 --- a/lib/internal/Magento/Framework/View/Element/AbstractBlock.php +++ b/lib/internal/Magento/Framework/View/Element/AbstractBlock.php @@ -1142,7 +1142,7 @@ protected function _getSidPlaceholder($cacheKey = null) * * @param string $name variable name * @param string|null $module optional module name - * @return string|false|array + * @return string|false */ public function getVar($name, $module = null) { From 2ae2aa92abf02e249edad9d37bbf019aad8204e6 Mon Sep 17 00:00:00 2001 From: gwharton <30697781+gwharton@users.noreply.github.com> Date: Fri, 7 Sep 2018 14:32:08 +0100 Subject: [PATCH 17/17] Update ConverterTest.php --- .../testsuite/Magento/Framework/Config/ConverterTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php b/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php index 767ab32f5d727..1d2e090d1923b 100644 --- a/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php +++ b/dev/tests/integration/testsuite/Magento/Framework/Config/ConverterTest.php @@ -1,6 +1,6 @@ objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager(); $this->converter = $this->objectManager->get(Converter::class); } -} \ No newline at end of file +}