Skip to content

FIX for issue #15501 - M2.2.4 missing meta title tag and doesn't show… #15532

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

Merged
magento-engcom-team merged 4 commits into from
Jun 22, 2018
Merged

FIX for issue #15501 - M2.2.4 missing meta title tag and doesn't show… #15532

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a38b84
FIX for issue #15501 - M2.2.4 missing meta title tag and doesn't show…
phoenix128 May 26, 2018
85ac3be
Merge remote-tracking branch 'upstream/2.2-develop' into issue-15501-…
phoenix128 Jun 4, 2018
432cf00
Refactor to avoid method_exists call
phoenix128 Jun 4, 2018
1413d65
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
7 changes: 3 additions & 4 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 @@ -113,10 +113,9 @@ private function preparePageMetadata(ResultPage $resultPage, $product)
{
$pageConfig = $resultPage->getConfig();

$title = $product->getMetaTitle();
if ($title) {
$pageConfig->getTitle()->set($title);
}
$metaTitle = $product->getMetaTitle();
$pageConfig->setMetaTitle($metaTitle);
$pageConfig->getTitle()->set($metaTitle ?: $product->getName());

$keyword = $product->getMetaKeyword();
$currentCategory = $this->_coreRegistry->registry('current_category');
Expand Down
8 changes: 8 additions & 0 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 @@ -339,6 +339,14 @@ public function getDescription()
return $this->metadata['description'];
}

/**
* @param string $title
*/
public function setMetaTitle($title)

@vkublytskyi vkublytskyi May 26, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please add type hint to the method to have more strict interface. Also to be consistent with other methods in this class it would be good to add corresponding getter

@phoenix128 phoenix128 May 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the whole class or just this method?

@ishakhsuvarov ishakhsuvarov May 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the class only

@phoenix128 phoenix128 Jun 4, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Uhm, but if I am going to refactor with strict types for the class, I risk to add a backward incompatiblity. Are you sure?

@ishakhsuvarov ishakhsuvarov Jun 4, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes for method only, I was saying something totally wrong on the previous comment. Sorry for the confusion.

{
$this->setMetadata('title', $title);
}

/**
* @param string $keywords
* @return void
Expand Down
4 changes: 3 additions & 1 deletion 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 @@ -136,7 +136,9 @@ public function renderMetadata()
protected function processMetadataContent($name, $content)
{
$method = 'get' . $this->string->upperCaseWords($name, '_', '');
if (method_exists($this->pageConfig, $method)) {

// 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)) {

@vkublytskyi vkublytskyi May 26, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not use simple !== operator here? Also probably would be good to refactor code and avoid magic with method_exists check

@phoenix128 phoenix128 May 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For readability, in case we need more methods to be skipped, but if you prefer I can refactor it with a single check

@ishakhsuvarov ishakhsuvarov May 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd also suggest to keep a single check. I assume it's not the best way to skip more methods anyway, so if we would need more then we also would need a different solution.

$content = $this->pageConfig->$method();
}
return $content;
Expand Down