Skip to content

Fix trim issue with empty page title #37385

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

Open
wants to merge 3 commits into
base: 2.4-develop
Choose a base branch
from
Open

Fix trim issue with empty page title #37385

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e9ae2d2
Fix trim issue with empty page title
sippsolutions Apr 17, 2023
869d8b6
Merge branch '2.4-develop' into fix-trim-issue
sippsolutions Dec 5, 2024
26f0708
Merge branch '2.4-develop' into fix-trim-issue
sippsolutions Dec 16, 2024
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
2 changes: 1 addition & 1 deletion app/code/Magento/Theme/Block/Html/Title.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public function getPageHeading()
{
$pageTitle = !empty($this->pageTitle) ? $this->pageTitle : $this->pageConfig->getTitle()->getShortHeading();

return $this->shouldTranslateTitle() ? __($pageTitle) : $pageTitle;
return $this->shouldTranslateTitle() ? __($pageTitle) : (string)$pageTitle;
Copy link
Member

Choose a reason for hiding this comment

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

Can we fix this elsewhere? The __() function also requires its argument to be a string, so the current suggestion isn't necessarily enough to fix the problem advertised.

Does the problem value come from $this->pageTitle or ...->getShortHeading()?
Perhaps we can improve the type information in the setPageTitle() method in this class, or fix the return value of ...->getShortHeading() if that's the source of the "bad" value.

Copy link
Member Author

Choose a reason for hiding this comment

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

__() does not require a string: gettype(__(null)->render()) = string so the given fix works perfectly for us.
Both pageTitle and getShortHeading are not typed and will not be in the near future.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the code has changed in the years since my comment. It looks like the Phrase constructor now does its own cast to a string for its argument.

magento2/lib/internal/Magento/Framework/Phrase/__.php

Lines 16 to 24 in 869d8b6

function __(...$argc)
{
$text = array_shift($argc);
if (!empty($argc) && is_array($argc[0])) {
$argc = $argc[0];
}
return new \Magento\Framework\Phrase($text, $argc);
}

magento2/lib/internal/Magento/Framework/Phrase.php

Lines 67 to 72 in 869d8b6

* @param string $text
* @param array $arguments
*/
public function __construct($text, array $arguments = [])
{
$this->text = (string)$text;

However, my comment regarding fixing the definition of the $pageTitle variable is still relevant here. We should not need to cast the value on this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

pageTitle is not typed and will probably never be. So it could be anything, especially when dealing with 3rd party code :-)
getPageHeading is not typed as well, so about your point fixing it somewhere else, I don't quite get where exactly.
We could use trim((string)$block->getPageHeading() in title.phtml, if you prefer that ...

We won't have the issue anywhere else, the only problem is that trim() expects a string as input, while Magento is not and probably will never be able to provide strict types in 2.x.

Copy link
Member

Choose a reason for hiding this comment

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

From a quick search just now, it looks like the property is typed (via a comment) as a string, so the root cause of the bug you're witnessing seems to be that getShortHeading() returns mixed and not string. Can we fix this in that method instead?

magento2/lib/internal/Magento/Framework/View/Page/Title.php

Line 88 in 869d8b6

* @return mixed

Copy link
Member Author

Choose a reason for hiding this comment

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

$this->pageTitle can be null as well.
See for example \Magento\Catalog\Helper\Product\View::preparePageMetadata where $pageMainTitle->setPageTitle($product->getName()) is used, while $product could possibly have a null name if imported via a wrongly formatted CSV-file by the customer (a case we experienced sometimes already)

I think there are too many possibilities all which only could be resolved by typing all corresponding methods and properties, which imo has a too big impact on stability, thinking about the thousands of extensions that are out there that may probably stop working after we enforce a string type on e.g. setPageTitle. The casting to string was the least risky option in my head, but feel free to adjust the PR if you feel other.

}

/**
Expand Down