Fixed issue where removing breadcrumbs removes the page title #29437

realchriswells · 2020-08-07T09:46:52Z

Description (*)

There was an issue in the 2.1 branch where if you removed the breadcrumbs via XML:
<referenceBlock name="breadcrumbs" remove="true" />, the page title would also be removed, leaving behind empty <title> tags in the <head>. This fixed in the 2.1 branch, but it seems that some regression has occurred and it is back. I'm reapplying that code change.

Related Pull Requests

The original PR is here.

Manual testing scenarios (*)

Remove breadcrumbs block in XML
Go to the category view or product view page
Check Page title in browser tab

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 are green)

Resolved issues:

resolves [Issue] Fixed issue where removing breadcrumbs removes the page title #29651: Fixed issue where removing breadcrumbs removes the page title

m2-assistant · 2020-08-07T09:46:55Z

Hi @realchriswells. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

realchriswells · 2020-08-07T10:33:32Z

@magento run all tests

realchriswells · 2020-08-07T13:40:27Z

@magento run Static Tests

rogyar

Sorry, cannot reproduce this issue on the 2.4-develop branch.

Steps:

Create a custom module
Use catalog_product_view.xml and add <referenceBlock name="breadcrumbs" remove="true"/>
Check the product details page

Result:

No breadcrumbs on the page
<title> tag is present and contains a correct value

realchriswells · 2020-08-07T14:05:23Z

Hi @rogyar

Apologies, this just seems to affect the category pages. Please try the updated steps to replicate below.

The steps I took to replicate the issue were:

Install 2.4 with sample data
Create new theme based on Magento/blank
Set theme as current
Create folder Magento_Catalog in theme
Create layout/catalog_category_view.xml:

<page xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:View/Layout/etc/page_configuration.xsd">
    <body>
        <referenceBlock name="breadcrumbs" remove="true" />
    </body>
</page>

Clear caches and ensure theme is built
Go to a category page and notice that the breadcrumb has gone, but the <title> tag is empty.

rogyar · 2020-08-09T14:32:39Z

Hi, @realchriswells. Thanks for the clarification. I'm able to reproduce it for the category view page with breadcrumbs block removed in the XML layout.

I've double-checked the original PR and looks like the changes were not ported to 2.2/2.3 for some reason. Frankly speaking, the fix looks like an "ad-hoc" solution. But, on the other hand, taking the page title from breadcrumbs (the current solution) is even more questionable. I believe it has been implemented in this way for a reason, so let's leave it as it is.

According to the definition of done, all changes should be covered by autotests. I would ask you to provide a simple test for this case. You may extend the existing test

magento2/dev/tests/integration/testsuite/Magento/Catalog/Block/BreadcrumbsTest.php

Line 56 in f4db7d4

public function testCategoriesSequence(): void

You may remove the breadcrumbs block using $this->layout and make sure that the title is not empty in the result output.

Thank you!

sidolov · 2020-08-18T17:04:18Z

@magento create issue

sidolov · 2020-08-26T17:22:36Z

@realchriswells I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

m2-assistant · 2020-08-26T17:22:38Z

Hi @realchriswells, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

realchriswells · 2020-10-05T12:45:20Z

I wish to continue with this.
Just getting the tests sorted.

realchriswells · 2020-11-02T13:51:52Z

@engcom-Charlie @rogyar
Please could this be reopened?
Also, is there anyone who is able to help me write these tests as they are proving to be difficult for me to write.

m2-assistant · 2020-11-02T13:58:10Z

Hi @realchriswells. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

chickenland · 2020-11-02T15:16:36Z

@magento run all tests

realchriswells · 2020-11-04T10:27:03Z

@rogyar Is it possible you or someone else on here could help me out with writing the tests, I'm out of my depth on those.

realchriswells · 2020-11-06T08:48:08Z

The other day there was a post on here confirming that a manual test was completed and accepted, but then it disappeared.
What is the next step for this PR?

rogyar · 2020-11-10T10:43:08Z

app/code/Magento/Catalog/Block/Breadcrumbs.php

@@ -4,6 +4,8 @@
 * See COPYING.txt for license details.
 */

+// @codingStandardsIgnoreFile


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

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?

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.

realchriswells · 2020-11-10T14:29:10Z

@magento run Static Tests

realchriswells · 2020-11-10T15:14:11Z

@magento run all tests

realchriswells · 2020-12-07T14:10:15Z

@rogyar This seems to have stalled. Are you able to tell me where things need to be to get this over the line?

rogyar · 2020-12-12T14:21:27Z

Hi @realchriswells as we have discussed in Slack, the auto-test coverage is still required.

realchriswells · 2021-03-12T16:31:28Z

@magento run all tests

Fixed issue where removing breadcrumbs removes the page title

b7e0198

ghost added this to Pending Review in Pull Requests Dashboard Aug 7, 2020

ghost added the Progress: pending review label Aug 7, 2020

magento-engcom-team added Component: Catalog Release Line: 2.4 Partner: Pinpoint partners-contribution Pull Request is created by Magento Partner labels Aug 7, 2020

rogyar self-assigned this Aug 7, 2020

rogyar requested changes Aug 7, 2020

View reviewed changes

ghost moved this from Pending Review to Changes Requested in Pull Requests Dashboard Aug 7, 2020

ghost added Progress: needs update and removed Progress: pending review labels Aug 7, 2020

rogyar added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Aug 9, 2020

engcom-Charlie self-assigned this Aug 17, 2020

sidolov added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Aug 18, 2020

m2-assistant bot mentioned this pull request Aug 18, 2020

[Issue] Fixed issue where removing breadcrumbs removes the page title #29651

Closed

4 tasks

sidolov closed this Aug 26, 2020

ghost removed this from Changes Requested in Pull Requests Dashboard Aug 26, 2020

Streamlining the _prepareLayout() method

1cc621c

engcom-Charlie reopened this Nov 2, 2020

ghost added this to Changes Requested in Pull Requests Dashboard Nov 2, 2020

engcom-Bravo self-assigned this Nov 4, 2020

rogyar requested changes Nov 10, 2020

View reviewed changes

Bringing Breadcrumbs.php in line with PSR Magento standards

a46e34a

m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Requests Dashboard Nov 11, 2020

m2-community-project bot added Progress: review and removed Progress: needs update labels Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue where removing breadcrumbs removes the page title #29437

Fixed issue where removing breadcrumbs removes the page title #29437

realchriswells commented Aug 7, 2020 •

edited by m2-assistant bot

m2-assistant bot commented Aug 7, 2020

realchriswells commented Aug 7, 2020

realchriswells commented Aug 7, 2020

rogyar left a comment

realchriswells commented Aug 7, 2020 •

edited

rogyar commented Aug 9, 2020

sidolov commented Aug 18, 2020

sidolov commented Aug 26, 2020

m2-assistant bot commented Aug 26, 2020

realchriswells commented Oct 5, 2020

realchriswells commented Nov 2, 2020

m2-assistant bot commented Nov 2, 2020

chickenland commented Nov 2, 2020

realchriswells commented Nov 4, 2020

realchriswells commented Nov 6, 2020

rogyar Nov 10, 2020

realchriswells Nov 11, 2020

rogyar Nov 16, 2020

realchriswells commented Nov 10, 2020

realchriswells commented Nov 10, 2020

realchriswells commented Dec 7, 2020

rogyar commented Dec 12, 2020

realchriswells commented Mar 12, 2021

Fixed issue where removing breadcrumbs removes the page title #29437

Are you sure you want to change the base?

Fixed issue where removing breadcrumbs removes the page title #29437

Conversation

realchriswells commented Aug 7, 2020 • edited by m2-assistant bot

Description (*)

Related Pull Requests

Manual testing scenarios (*)

Contribution checklist (*)

Resolved issues:

m2-assistant bot commented Aug 7, 2020

realchriswells commented Aug 7, 2020

realchriswells commented Aug 7, 2020

rogyar left a comment

Choose a reason for hiding this comment

realchriswells commented Aug 7, 2020 • edited

rogyar commented Aug 9, 2020

sidolov commented Aug 18, 2020

sidolov commented Aug 26, 2020

m2-assistant bot commented Aug 26, 2020

realchriswells commented Oct 5, 2020

realchriswells commented Nov 2, 2020

m2-assistant bot commented Nov 2, 2020

chickenland commented Nov 2, 2020

realchriswells commented Nov 4, 2020

realchriswells commented Nov 6, 2020

rogyar Nov 10, 2020

Choose a reason for hiding this comment

realchriswells Nov 11, 2020

Choose a reason for hiding this comment

rogyar Nov 16, 2020

Choose a reason for hiding this comment

realchriswells commented Nov 10, 2020

realchriswells commented Nov 10, 2020

realchriswells commented Dec 7, 2020

rogyar commented Dec 12, 2020

realchriswells commented Mar 12, 2021

realchriswells commented Aug 7, 2020 •

edited by m2-assistant bot

realchriswells commented Aug 7, 2020 •

edited