From 8b963a7ad59c1be44e66ec3247687fe6948e7191 Mon Sep 17 00:00:00 2001 From: Riccardo Tempesta Date: Sat, 26 May 2018 12:27:53 +0200 Subject: [PATCH 1/3] Fixed issue on set of meta title --- app/code/Magento/Catalog/Helper/Product/View.php | 9 +++------ lib/internal/Magento/Framework/View/Page/Config.php | 8 ++++++++ .../Magento/Framework/View/Page/Config/Renderer.php | 3 ++- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/code/Magento/Catalog/Helper/Product/View.php b/app/code/Magento/Catalog/Helper/Product/View.php index 5753910c125d2..1509e489aee3b 100644 --- a/app/code/Magento/Catalog/Helper/Product/View.php +++ b/app/code/Magento/Catalog/Helper/Product/View.php @@ -112,12 +112,9 @@ private function preparePageMetadata(ResultPage $resultPage, $product) $pageLayout = $resultPage->getLayout(); $pageConfig = $resultPage->getConfig(); - $title = $product->getMetaTitle(); - if ($title) { - $pageConfig->getTitle()->set($title); - } else { - $pageConfig->getTitle()->set($product->getName()); - } + $metaTitle = $product->getMetaTitle(); + $pageConfig->setMetaTitle($metaTitle); + $pageConfig->getTitle()->set($metaTitle ?: $product->getName()); $keyword = $product->getMetaKeyword(); $currentCategory = $this->_coreRegistry->registry('current_category'); diff --git a/lib/internal/Magento/Framework/View/Page/Config.php b/lib/internal/Magento/Framework/View/Page/Config.php index d42d30e35cc5b..dd8fcd42b5fc8 100644 --- a/lib/internal/Magento/Framework/View/Page/Config.php +++ b/lib/internal/Magento/Framework/View/Page/Config.php @@ -340,6 +340,14 @@ public function getDescription() return $this->metadata['description']; } + /** + * @param string $title + */ + public function setMetaTitle($title) + { + $this->setMetadata('title', $title); + } + /** * @param string $keywords * @return void diff --git a/lib/internal/Magento/Framework/View/Page/Config/Renderer.php b/lib/internal/Magento/Framework/View/Page/Config/Renderer.php index 93c8c5c338627..58c010bcdb279 100644 --- a/lib/internal/Magento/Framework/View/Page/Config/Renderer.php +++ b/lib/internal/Magento/Framework/View/Page/Config/Renderer.php @@ -142,7 +142,8 @@ protected function processMetadataContent($name, $content) } return $content; } - if (method_exists($this->pageConfig, $method)) { + // We skip title, because PageConfig::getTitle() refers to the tag and not to meta title. + if (!in_array($name, ['title']) && method_exists($this->pageConfig, $method)) { $content = $this->pageConfig->$method(); } return $content; From eafd791eae87748a912d58a58aa032f3cd3b8936 Mon Sep 17 00:00:00 2001 From: Riccardo Tempesta <riccardo.tempesta@magespecialist.it> Date: Mon, 4 Jun 2018 19:06:11 +0200 Subject: [PATCH 2/3] Removed extra code on renderer --- .../Magento/Framework/View/Page/Config.php | 52 +++++++++++++++++++ .../Framework/View/Page/Config/Renderer.php | 19 +++---- 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/lib/internal/Magento/Framework/View/Page/Config.php b/lib/internal/Magento/Framework/View/Page/Config.php index dd8fcd42b5fc8..9341a51f263f4 100644 --- a/lib/internal/Magento/Framework/View/Page/Config.php +++ b/lib/internal/Magento/Framework/View/Page/Config.php @@ -7,6 +7,7 @@ namespace Magento\Framework\View\Page; use Magento\Framework\App; +use Magento\Framework\Exception\LocalizedException; use Magento\Framework\View; /** @@ -34,6 +35,14 @@ class Config const ELEMENT_TYPE_HEAD = 'head'; /**#@-*/ + const META_DESCRIPTION = 'description'; + const META_CONTENT_TYPE = 'content_type'; + const META_MEDIA_TYPE = 'media_type'; + const META_CHARSET = 'charset'; + const META_TITLE = 'title'; + const META_KEYWORDS = 'keywords'; + const META_ROBOTS = 'robots'; + /** * Constant body attribute class */ @@ -340,6 +349,34 @@ public function getDescription() return $this->metadata['description']; } + /** + * Get rendered metadata + * @param string $fieldName + * @return string + * @throws LocalizedException + */ + public function getRenderedMetaTagValue(string $fieldName) + { + switch ($fieldName) { + case self::META_DESCRIPTION: + return $this->getDescription(); + case self::META_CONTENT_TYPE: + return $this->getContentType(); + case self::META_MEDIA_TYPE: + return $this->getMediaType(); + case self::META_CHARSET: + return $this->getCharset(); + case self::META_KEYWORDS: + return $this->getKeywords(); + case self::META_ROBOTS: + return $this->getRobots(); + case self::META_TITLE: + return $this->getMetaTitle(); + default: + throw new LocalizedException(__('No rendered meta function for %1', $fieldName)); + } + } + /** * @param string $title */ @@ -348,6 +385,21 @@ public function setMetaTitle($title) $this->setMetadata('title', $title); } + /** + * Retrieve meta title + * + * @return string + */ + public function getMetaTitle() + { + $this->build(); + if (empty($this->metadata['title'])) { + return ''; + } + + return $this->metadata['title']; + } + /** * @param string $keywords * @return void diff --git a/lib/internal/Magento/Framework/View/Page/Config/Renderer.php b/lib/internal/Magento/Framework/View/Page/Config/Renderer.php index 58c010bcdb279..f251580441b2f 100644 --- a/lib/internal/Magento/Framework/View/Page/Config/Renderer.php +++ b/lib/internal/Magento/Framework/View/Page/Config/Renderer.php @@ -3,8 +3,10 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ + namespace Magento\Framework\View\Page\Config; +use Magento\Framework\Exception\LocalizedException; use Magento\Framework\View\Asset\GroupedCollection; use Magento\Framework\View\Page\Config; @@ -131,22 +133,15 @@ public function renderMetadata() /** * @param string $name * @param string $content - * @return mixed + * @return string */ protected function processMetadataContent($name, $content) { - $method = 'get' . $this->string->upperCaseWords($name, '_', ''); - if ($name === 'title') { - if (!$content) { - $content = $this->escaper->escapeHtml($this->pageConfig->$method()->get()); - } - return $content; - } - // We skip title, because PageConfig::getTitle() refers to the tag <title> and not to meta title. - if (!in_array($name, ['title']) && method_exists($this->pageConfig, $method)) { - $content = $this->pageConfig->$method(); + try { + return $this->pageConfig->getRenderedMetaTagValue((string) $name); + } catch (LocalizedException $e) { + return (string) $content; } - return $content; } /** From 6342e5d9f16a9ebfc8f7aae9149bf9dec58586c6 Mon Sep 17 00:00:00 2001 From: Riccardo Tempesta <riccardo.tempesta@magespecialist.it> Date: Tue, 5 Jun 2018 18:38:13 +0200 Subject: [PATCH 3/3] FIX failing tests and PageConfig refactor --- .../Magento/Framework/View/Page/Config.php | 88 +++++++------------ .../Framework/View/Page/Config/Renderer.php | 42 ++++++--- .../Test/Unit/Page/Config/RendererTest.php | 9 +- 3 files changed, 66 insertions(+), 73 deletions(-) diff --git a/lib/internal/Magento/Framework/View/Page/Config.php b/lib/internal/Magento/Framework/View/Page/Config.php index 9341a51f263f4..b9f72e49b28a7 100644 --- a/lib/internal/Magento/Framework/View/Page/Config.php +++ b/lib/internal/Magento/Framework/View/Page/Config.php @@ -7,7 +7,7 @@ namespace Magento\Framework\View\Page; use Magento\Framework\App; -use Magento\Framework\Exception\LocalizedException; +use Magento\Framework\App\Area; use Magento\Framework\View; /** @@ -42,6 +42,7 @@ class Config const META_TITLE = 'title'; const META_KEYWORDS = 'keywords'; const META_ROBOTS = 'robots'; + const META_X_UI_COMPATIBLE = 'x_ua_compatible'; /** * Constant body attribute class @@ -254,7 +255,7 @@ public function getMetadata() */ public function setContentType($contentType) { - $this->setMetadata('content_type', $contentType); + $this->setMetadata(self::META_CONTENT_TYPE, $contentType); } /** @@ -265,10 +266,10 @@ public function setContentType($contentType) public function getContentType() { $this->build(); - if (strtolower($this->metadata['content_type']) === 'auto') { - $this->metadata['content_type'] = $this->getMediaType() . '; charset=' . $this->getCharset(); + if (strtolower($this->metadata[self::META_CONTENT_TYPE]) === 'auto') { + $this->metadata[self::META_CONTENT_TYPE] = $this->getMediaType() . '; charset=' . $this->getCharset(); } - return $this->metadata['content_type']; + return $this->metadata[self::META_CONTENT_TYPE]; } /** @@ -277,7 +278,7 @@ public function getContentType() */ public function setMediaType($mediaType) { - $this->setMetadata('media_type', $mediaType); + $this->setMetadata(self::META_MEDIA_TYPE, $mediaType); } /** @@ -288,13 +289,13 @@ public function setMediaType($mediaType) public function getMediaType() { $this->build(); - if (empty($this->metadata['media_type'])) { - $this->metadata['media_type'] = $this->scopeConfig->getValue( + if (empty($this->metadata[self::META_MEDIA_TYPE])) { + $this->metadata[self::META_MEDIA_TYPE] = $this->scopeConfig->getValue( 'design/head/default_media_type', \Magento\Store\Model\ScopeInterface::SCOPE_STORE ); } - return $this->metadata['media_type']; + return $this->metadata[self::META_MEDIA_TYPE]; } /** @@ -303,7 +304,7 @@ public function getMediaType() */ public function setCharset($charset) { - $this->setMetadata('charset', $charset); + $this->setMetadata(self::META_CHARSET, $charset); } /** @@ -314,13 +315,13 @@ public function setCharset($charset) public function getCharset() { $this->build(); - if (empty($this->metadata['charset'])) { - $this->metadata['charset'] = $this->scopeConfig->getValue( + if (empty($this->metadata[self::META_CHARSET])) { + $this->metadata[self::META_CHARSET] = $this->scopeConfig->getValue( 'design/head/default_charset', \Magento\Store\Model\ScopeInterface::SCOPE_STORE ); } - return $this->metadata['charset']; + return $this->metadata[self::META_CHARSET]; } /** @@ -329,7 +330,7 @@ public function getCharset() */ public function setDescription($description) { - $this->setMetadata('description', $description); + $this->setMetadata(self::META_DESCRIPTION, $description); } /** @@ -340,41 +341,13 @@ public function setDescription($description) public function getDescription() { $this->build(); - if (empty($this->metadata['description'])) { - $this->metadata['description'] = $this->scopeConfig->getValue( + if (empty($this->metadata[self::META_DESCRIPTION])) { + $this->metadata[self::META_DESCRIPTION] = $this->scopeConfig->getValue( 'design/head/default_description', \Magento\Store\Model\ScopeInterface::SCOPE_STORE ); } - return $this->metadata['description']; - } - - /** - * Get rendered metadata - * @param string $fieldName - * @return string - * @throws LocalizedException - */ - public function getRenderedMetaTagValue(string $fieldName) - { - switch ($fieldName) { - case self::META_DESCRIPTION: - return $this->getDescription(); - case self::META_CONTENT_TYPE: - return $this->getContentType(); - case self::META_MEDIA_TYPE: - return $this->getMediaType(); - case self::META_CHARSET: - return $this->getCharset(); - case self::META_KEYWORDS: - return $this->getKeywords(); - case self::META_ROBOTS: - return $this->getRobots(); - case self::META_TITLE: - return $this->getMetaTitle(); - default: - throw new LocalizedException(__('No rendered meta function for %1', $fieldName)); - } + return $this->metadata[self::META_DESCRIPTION]; } /** @@ -382,7 +355,7 @@ public function getRenderedMetaTagValue(string $fieldName) */ public function setMetaTitle($title) { - $this->setMetadata('title', $title); + $this->setMetadata(self::META_TITLE, $title); } /** @@ -393,11 +366,11 @@ public function setMetaTitle($title) public function getMetaTitle() { $this->build(); - if (empty($this->metadata['title'])) { + if (empty($this->metadata[self::META_TITLE])) { return ''; } - return $this->metadata['title']; + return $this->metadata[self::META_TITLE]; } /** @@ -406,7 +379,7 @@ public function getMetaTitle() */ public function setKeywords($keywords) { - $this->setMetadata('keywords', $keywords); + $this->setMetadata(self::META_KEYWORDS, $keywords); } /** @@ -417,13 +390,13 @@ public function setKeywords($keywords) public function getKeywords() { $this->build(); - if (empty($this->metadata['keywords'])) { - $this->metadata['keywords'] = $this->scopeConfig->getValue( + if (empty($this->metadata[self::META_KEYWORDS])) { + $this->metadata[self::META_KEYWORDS] = $this->scopeConfig->getValue( 'design/head/default_keywords', \Magento\Store\Model\ScopeInterface::SCOPE_STORE ); } - return $this->metadata['keywords']; + return $this->metadata[self::META_KEYWORDS]; } /** @@ -432,27 +405,28 @@ public function getKeywords() */ public function setRobots($robots) { - $this->setMetadata('robots', $robots); + $this->setMetadata(self::META_ROBOTS, $robots); } /** * Retrieve URL to robots file * * @return string + * @throws \Magento\Framework\Exception\LocalizedException */ public function getRobots() { - if ($this->getAreaResolver()->getAreaCode() !== 'frontend') { + if ($this->getAreaResolver()->getAreaCode() !== Area::AREA_FRONTEND) { return 'NOINDEX,NOFOLLOW'; } $this->build(); - if (empty($this->metadata['robots'])) { - $this->metadata['robots'] = $this->scopeConfig->getValue( + if (empty($this->metadata[self::META_ROBOTS])) { + $this->metadata[self::META_ROBOTS] = $this->scopeConfig->getValue( 'design/search_engine_robots/default_robots', \Magento\Store\Model\ScopeInterface::SCOPE_STORE ); } - return $this->metadata['robots']; + return $this->metadata[self::META_ROBOTS]; } /** diff --git a/lib/internal/Magento/Framework/View/Page/Config/Renderer.php b/lib/internal/Magento/Framework/View/Page/Config/Renderer.php index f251580441b2f..183f8d34ee41f 100644 --- a/lib/internal/Magento/Framework/View/Page/Config/Renderer.php +++ b/lib/internal/Magento/Framework/View/Page/Config/Renderer.php @@ -23,7 +23,7 @@ class Renderer implements RendererInterface protected $assetTypeOrder = ['css', 'ico', 'js']; /** - * @var \Magento\Framework\View\Page\Config + * @var Config */ protected $pageConfig; @@ -53,7 +53,7 @@ class Renderer implements RendererInterface protected $urlBuilder; /** - * @param \Magento\Framework\View\Page\Config $pageConfig + * @param Config $pageConfig * @param \Magento\Framework\View\Asset\MergeService $assetMergeService * @param \Magento\Framework\UrlInterface $urlBuilder * @param \Magento\Framework\Escaper $escaper @@ -137,10 +137,30 @@ public function renderMetadata() */ protected function processMetadataContent($name, $content) { - try { - return $this->pageConfig->getRenderedMetaTagValue((string) $name); - } catch (LocalizedException $e) { - return (string) $content; + switch ($name) { + case Config::META_DESCRIPTION: + return $this->pageConfig->getDescription(); + + case Config::META_CONTENT_TYPE: + return $this->pageConfig->getContentType(); + + case Config::META_MEDIA_TYPE: + return $this->pageConfig->getMediaType(); + + case Config::META_CHARSET: + return $this->pageConfig->getCharset(); + + case Config::META_KEYWORDS: + return $this->pageConfig->getKeywords(); + + case Config::META_ROBOTS: + return $this->pageConfig->getRobots(); + + case Config::META_TITLE: + return $this->pageConfig->getMetaTitle(); + + default: + return $content; } } @@ -155,19 +175,19 @@ protected function getMetadataTemplate($name) } switch ($name) { - case 'charset': + case Config::META_CHARSET: $metadataTemplate = '<meta charset="%content"/>' . "\n"; break; - case 'content_type': + case Config::META_CONTENT_TYPE: $metadataTemplate = '<meta http-equiv="Content-Type" content="%content"/>' . "\n"; break; - case 'x_ua_compatible': + case Config::META_X_UI_COMPATIBLE: $metadataTemplate = '<meta http-equiv="X-UA-Compatible" content="%content"/>' . "\n"; break; - case 'media_type': + case Config::META_MEDIA_TYPE: $metadataTemplate = false; break; @@ -354,7 +374,7 @@ protected function renderAssetHtml(\Magento\Framework\View\Asset\PropertyGroup $ ); $result .= sprintf($template, $asset->getUrl()); } - } catch (\Magento\Framework\Exception\LocalizedException $e) { + } catch (LocalizedException $e) { $this->logger->critical($e); $result .= sprintf($template, $this->urlBuilder->getUrl('', ['_direct' => 'core/index/notFound'])); } diff --git a/lib/internal/Magento/Framework/View/Test/Unit/Page/Config/RendererTest.php b/lib/internal/Magento/Framework/View/Test/Unit/Page/Config/RendererTest.php index 1f110a9ec19b5..adb7c4f4ddd86 100644 --- a/lib/internal/Magento/Framework/View/Test/Unit/Page/Config/RendererTest.php +++ b/lib/internal/Magento/Framework/View/Test/Unit/Page/Config/RendererTest.php @@ -157,15 +157,14 @@ public function testRenderMetadata() . '<meta http-equiv="X-UA-Compatible" content="x_ua_compatible_value"/>' . "\n" . '<meta property="og:video:secure_url" content="secure_url"/>' . "\n"; - $this->stringMock->expects($this->at(0)) - ->method('upperCaseWords') - ->with('charset', '_', '') - ->willReturn('Charset'); - $this->pageConfigMock->expects($this->once()) ->method('getCharset') ->willReturn($metadataValueCharset); + $this->pageConfigMock->expects($this->once()) + ->method('getContentType') + ->willReturn('content_type_value'); + $this->pageConfigMock ->expects($this->once()) ->method('getMetadata')