Skip to content
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.

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

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

Conversation

phoenix128
Copy link
Contributor

@phoenix128 phoenix128 commented May 26, 2018

FIX for issue #15501 - M2.2.4 missing meta title tag and doesn't show product name if meta title is empty

Description

The current Magento behaviour does not allow to specify a title meta tag and the "Meta Title" option in the product's page is binded to the page's title.

This PR adds the meta tile tag and allows to use title and meta title independently.

Fixed Issues (if relevant)

  1. M2.2.4 missing meta title tag and doesn't show product name if meta title is empty #15501: M2.2.4 missing meta title tag and doesn't show product name if meta title is empty

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

…'t show product name if meta title is empty
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)) {

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

Copy link
Contributor Author

@phoenix128 phoenix128 May 26, 2018

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

Copy link
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.

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

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

Copy link
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?

Copy link
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

Copy link
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?

Copy link
Contributor

@ishakhsuvarov ishakhsuvarov Jun 4, 2018

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.

@ihor-sviziev ihor-sviziev added partners-contribution Pull Request is created by Magento Partner and removed partner-contribution labels Jun 5, 2018
@phoenix128
Copy link
Contributor Author

@vkublytskyi , it should be ok now. Is there anything I should do on it?

@vkublytskyi
Copy link

@phoenix128 For me it looks good. Thank you, your PR should be merged soon

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @phoenix128. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.6 release.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants