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 meta title property #11368

Merged
merged 3 commits into from
May 21, 2018

Conversation

slackerzz
Copy link
Member

@slackerzz slackerzz commented Oct 11, 2017

Description

If, inside a controller you try to set metadata title with:

$resultPage->getConfig()->setMetadata('title', 'some meta title');

you will get an error on frontend:

Recoverable Error: Object of class Magento\Framework\View\Page\Title could not be converted to string...

becouse processMetadataContent will return an istance of Magento\Framework\View\Page\Title instead of a string.

Fixed Issues (if relevant)

  1. Unable to render page when 'meta title' page config param is set #2956: Unable to render page when 'meta title' page config param is set

Manual testing scenarios

Scenario 1

  1. Create a controller
  2. Inside the execute function add:
$resultPage = $this->resultPageFactory->create();
$resultPage->getConfig()->getTitle->set('my title');
$resultPage->getConfig()->setMetadata('title', 'metatitle');
return $resultPage;
  1. As result you will have in your page:
<title>my title</title>
<meta name="title" content="metatitle"/>

Scenario 2

  1. Create a controller
  2. Inside the execute function add:
$resultPage = $this->resultPageFactory->create();
$resultPage->getConfig()->getTitle->set('my title');
return $resultPage;
  1. As result you will have in your page:
<title>my title</title>
<meta name="title" content="my title"/>

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)

@@ -136,6 +136,12 @@ public function renderMetadata()
protected function processMetadataContent($name, $content)
{
$method = 'get' . $this->string->upperCaseWords($name, '_', '');
if($name === 'title') {
if (!$content) {
$content = $this->escaper->escapeHtml($this->pageConfig->$method()->get());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need escapeHtml here? Seems inconsistent with other meta data getters.

Maybe just add __toString to \Magento\Framework\View\Page\Title?

Copy link
Member Author

@slackerzz slackerzz Oct 11, 2017

Choose a reason for hiding this comment

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

Becouse in the same file at line 109 we have:

return '<title>' . $this->escaper->escapeHtml($this->pageConfig->getTitle()->get()) . '</title>' . "\n";

so i reproduced the same behaviour

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the original reason for that is that if someone enter some html inside the title is has to be escaped to not break the html

@slackerzz
Copy link
Member Author

tests are failing, i'll try to fix them

@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:32
@magento-engcom-team magento-engcom-team added Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Dec 25, 2017
@slackerzz
Copy link
Member Author

@magento-engcom-team any news?

@orlangur
Copy link
Contributor

Hi @slackerzz, changes looks good to me now but process has changed a bit since the time this PR was created.

We need to deliver any contribution into 2.2-develop first, could you please create a new PR targeting 2.2-develop leaving this one as is or change a target branch of this PR?

@slackerzz slackerzz changed the base branch from 2.3-develop to 2.2-develop March 13, 2018 13:48
@slackerzz slackerzz changed the base branch from 2.2-develop to 2.3-develop March 13, 2018 13:48
@slackerzz
Copy link
Member Author

Hi @orlangur, i'll create a new PR since changing the target branch generates a lot of conflicts

@magento-engcom-team
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend bugfix Partner: MageSpecialist partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants