Skip to content

[Forwardport] M2.2.4 missing meta title tag and doesn't show #16608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gelanivishal wants to merge 3 commits into from
Closed

[Forwardport] M2.2.4 missing meta title tag and doesn't show #16608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b963a7
Fixed issue on set of meta title
phoenix128 May 26, 2018
eafd791
Removed extra code on renderer
phoenix128 Jun 4, 2018
6342e5d
FIX failing tests and PageConfig refactor
phoenix128 Jun 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions app/code/Magento/Catalog/Helper/Product/View.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
84 changes: 59 additions & 25 deletions lib/internal/Magento/Framework/View/Page/Config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
namespace Magento\Framework\View\Page;

use Magento\Framework\App;
use Magento\Framework\App\Area;
use Magento\Framework\View;

/**
Expand Down Expand Up @@ -34,6 +35,15 @@ 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';
const META_X_UI_COMPATIBLE = 'x_ua_compatible';

/**
* Constant body attribute class
*/
Expand Down Expand Up @@ -245,7 +255,7 @@ public function getMetadata()
*/
public function setContentType($contentType)
{
$this->setMetadata('content_type', $contentType);
$this->setMetadata(self::META_CONTENT_TYPE, $contentType);
}

/**
Expand All @@ -256,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];
}

/**
Expand All @@ -268,7 +278,7 @@ public function getContentType()
*/
public function setMediaType($mediaType)
{
$this->setMetadata('media_type', $mediaType);
$this->setMetadata(self::META_MEDIA_TYPE, $mediaType);
}

/**
Expand All @@ -279,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];
}

/**
Expand All @@ -294,7 +304,7 @@ public function getMediaType()
*/
public function setCharset($charset)
{
$this->setMetadata('charset', $charset);
$this->setMetadata(self::META_CHARSET, $charset);
}

/**
Expand All @@ -305,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];
}

/**
Expand All @@ -320,7 +330,7 @@ public function getCharset()
*/
public function setDescription($description)
{
$this->setMetadata('description', $description);
$this->setMetadata(self::META_DESCRIPTION, $description);
}

/**
Expand All @@ -331,13 +341,36 @@ 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'];
return $this->metadata[self::META_DESCRIPTION];
}

/**
* @param string $title
*/
public function setMetaTitle($title)
{
$this->setMetadata(self::META_TITLE, $title);
}

/**
* Retrieve meta title
*
* @return string
*/
public function getMetaTitle()
{
$this->build();
if (empty($this->metadata[self::META_TITLE])) {
return '';
}

return $this->metadata[self::META_TITLE];
}

/**
Expand All @@ -346,7 +379,7 @@ public function getDescription()
*/
public function setKeywords($keywords)
{
$this->setMetadata('keywords', $keywords);
$this->setMetadata(self::META_KEYWORDS, $keywords);
}

/**
Expand All @@ -357,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];
}

/**
Expand All @@ -372,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];
}

/**
Expand Down
52 changes: 34 additions & 18 deletions lib/internal/Magento/Framework/View/Page/Config/Renderer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -21,7 +23,7 @@ class Renderer implements RendererInterface
protected $assetTypeOrder = ['css', 'ico', 'js'];

/**
* @var \Magento\Framework\View\Page\Config
* @var Config
*/
protected $pageConfig;

Expand Down Expand Up @@ -51,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
Expand Down Expand Up @@ -131,21 +133,35 @@ 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') {
Copy link
Copy Markdown
Contributor

@sidolov sidolov Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like additional logic for title attribute should be here, please, revert.

Copy link
Copy Markdown
Contributor Author

@gelanivishal gelanivishal Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sidolov Do you want to keep old logic for 2.3 ?

Copy link
Copy Markdown
Contributor

@sidolov sidolov Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gelanivishal , yes, this logic should be here due to bugfix

if (!$content) {
$content = $this->escaper->escapeHtml($this->pageConfig->$method()->get());
}
return $content;
}
if (method_exists($this->pageConfig, $method)) {
$content = $this->pageConfig->$method();
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;
}
return $content;
}

/**
Expand All @@ -159,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;

Expand Down Expand Up @@ -358,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']));
}
Expand Down
9 changes: 4 additions & 5 deletions lib/internal/Magento/Framework/View/Test/Unit/Page/Config/RendererTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down