Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The breadcrumbs in the Search module currently use 'Node' and 'User' as the text for the breadcrumbs, but should use 'Content' and 'Users' to match the terminology used in the tabs.
Comment | File | Size | Author |
---|---|---|---|
#27 | 2099391-add-test-27.patch | 1.02 KB | marthinal |
#27 | 2099391-add-test-27-without-previous-patch-should-fail.patch | 2.46 KB | marthinal |
#23 | 2099391-add-test-23-without-previous-patch-should-fail.patch | 2.28 KB | jhodgdon |
#23 | 2099391-add-test-23.patch | 857 bytes | jhodgdon |
#18 | after.png | 236 KB | marthinal |
Comments
Comment #1
jhodgdonGood catch, thanks for reporting it!
Comment #2
larowlanWill need a _title entry in the routing defaults
Comment #3
jhodgdonActually, this text mismatch is a UI bug.
Comment #4
dawehnerNote: this does retranslate the titles twice.
Comment #5
geodaniel CreditAttribution: geodaniel commentedComment #7
dawehnerRerolled.
Comment #9
marthinal CreditAttribution: marthinal commented7: 2099391.patch queued for re-testing.
Comment #11
jhodgdonThose look like real test failures (SearchPageText title fails)
Comment #12
marthinal CreditAttribution: marthinal commentedTest added.
Comment #13
marthinal CreditAttribution: marthinal commentedUpps without debug()
Comment #14
jhodgdonCan you please post a screen shot of what the Search pages for Node and User look like, with the browser title bar and breadcrumbs included, before and after the patch? Because it looks like the test was changed so that it is not expecting the word "Search" in the page title. That is not right -- I think we still want the page title to say "Search" somewhere in it, not "Content"? The only thing that should be changing is the breadcrumbs, right?
Comment #15
marthinal CreditAttribution: marthinal commented@jhodgdon Absolutely, you have the reason. So the problem is the patch and not the test.
Screenshots attached.
I'll take a look at this...
Thanks
Comment #16
dawehnerWe can indeed fix that without having to work on the tests.
Comment #17
jhodgdonCan we have another before/after screen shot please?
Comment #18
marthinal CreditAttribution: marthinal commentedThis is exactly what I was searching for :)
getTitle() method at TitleResolver.php was adding the same title for the breadcrumb and for the page title...
Screenshots attached.
Comment #19
jhodgdonOK then. It looks like this latest patch passes all the tests without any test modifications, and the before/after screen shots show that it does solve the issue that was reported here. Let's get it in.
Thanks!
Comment #20
Xano16: 2099391.patch queued for re-testing.
Comment #21
alexpottCommitted 71d9179 and pushed to 8.x. Thanks!
Comment #22
tim.plunkettI almost accidentally reverted this in another issue that I was rebasing. Why wasn't test coverage added here? This was a regression after all.
If someone wants to open a follow-up, that's fine.
Comment #23
jhodgdonI guess we should have a test that verifies the word "Node" does not appear on the screen when you go to the search page?
I think this should do it. I've uploaded both a patch with a new test, and a second patch that also comments out the new lines added in this issue, which should fail if this is a valid new test.
Comment #25
jhodgdonThat test did not fail on the right lines. :( Perhaps breadcrumbs are not visible during the test. We should probably open a new issue and I don't have time to work on it today.
Comment #26
marthinal CreditAttribution: marthinal commented23: 2099391-add-test-23-without-previous-patch-should-fail.patch queued for re-testing.
Comment #27
marthinal CreditAttribution: marthinal commentedI have no fails at the views test...
Changed the place for the assertions.
Let's take a look at the test.
Comment #29
dawehnerPerfect!
Comment #30
jhodgdonThanks for fixing that marthinal! :) I agree, this test is working as we hoped and should be committed.
Comment #31
catchCommitted/pushed to 8.x, thanks!