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.

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

Fixed issue where removing breadcrumbs removes the page title #29437

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

Fixed issue where removing breadcrumbs removes the page title #29437

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7e0198
Fixed issue where removing breadcrumbs removes the page title
Aug 7, 2020
1cc621c
Streamlining the _prepareLayout() method
Nov 2, 2020
a46e34a
Bringing Breadcrumbs.php in line with PSR Magento standards
Nov 10, 2020
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
20 changes: 8 additions & 12 deletions app/code/Magento/Catalog/Block/Breadcrumbs.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* See COPYING.txt for license details.
*/

// @codingStandardsIgnoreFile
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, don't use this annotation. Even if this page contains some legacy approaches, they should be visible.
On the other hand, I see that some of your edits contain changes that don't meet the PSR coding standards (line length exceeds 120 chars). Please, remove this annotation and run the static tests once again.

Thank you

Copy link
Author

Choose a reason for hiding this comment

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

I've done this now, and fixed the PSR coding standards issue you referenced.

As of now it is passing all tests bar the Functional Tests. What needs to be done next?

Copy link
Contributor

Choose a reason for hiding this comment

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

By adding this directive you don't "fix" the coding standard issues but tell the system to ignore them :)
Using such directives is not allowed in most cases (including this one). If there are failing static tests, every issue that causes the failing test should be addressed by adjusting the code for meeting the standards.

Thank you.


/**
* Catalog breadcrumbs
*/
Expand Down Expand Up @@ -41,11 +43,7 @@ public function __construct(Context $context, Data $catalogData, array $data = [
*/
public function getTitleSeparator($store = null)
{
$separator = (string)$this->_scopeConfig->getValue(
'catalog/seo/title_separator',
\Magento\Store\Model\ScopeInterface::SCOPE_STORE,
$store
);
$separator = (string)$this->_scopeConfig->getValue('catalog/seo/title_separator', \Magento\Store\Model\ScopeInterface::SCOPE_STORE, $store);
return ' ' . $separator . ' ';
}

Expand All @@ -56,6 +54,8 @@ public function getTitleSeparator($store = null)
*/
protected function _prepareLayout()
{
$path = $this->_catalogData->getBreadcrumbPath();

if ($breadcrumbsBlock = $this->getLayout()->getBlock('breadcrumbs')) {
$breadcrumbsBlock->addCrumb(
'home',
Expand All @@ -65,17 +65,13 @@ protected function _prepareLayout()
'link' => $this->_storeManager->getStore()->getBaseUrl()
]
);

$title = [];
$path = $this->_catalogData->getBreadcrumbPath();

foreach ($path as $name => $breadcrumb) {
$breadcrumbsBlock->addCrumb($name, $breadcrumb);
$title[] = $breadcrumb['label'];
}

$this->pageConfig->getTitle()->set(join($this->getTitleSeparator(), array_reverse($title)));
}

$title = array_column($path, 'label');
$this->pageConfig->getTitle()->set(join($this->getTitleSeparator(), array_reverse($title)));
return parent::_prepareLayout();
}
}