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.Follow-up to #2306407: Remove breadcrumb from page template.
The change there causes contrib tests using FieldUiTestTrait to fail.
Two things are needed I think:
- a separate change record to state that tests using FieldUiTestTrait need to be updated to enable the breadcrumb block
- documentation needs to be added to FieldUiTestTrait stating this too.
Eg, CommentNonNodeTest uses the trait, and does this:
protected function setUp() {
parent::setUp();
$this->drupalPlaceBlock('system_breadcrumb_block');
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2417517-17-21.drupal.field-ui-test-trait.interdiff.txt | 2.49 KB | joachim |
| #21 | 2417517-21.drupal.field-ui-test-trait.patch | 2.55 KB | joachim |
| #17 | interdiff-2417517-15-17.txt | 1.36 KB | kfitz |
| #17 | 2417517-17-test-only.patch | 1.36 KB | kfitz |
Comments
Comment #1
joachim CreditAttribution: joachim commentedComment #2
joachim CreditAttribution: joachim commented"ack -g TestTrait | ack -x breadcrumb" shows that only this trait looks at breadcrumbs.
Comment #3
joachim CreditAttribution: joachim commentedAs for the change record, can this be added to the CR for the issue that changed this, https://www.drupal.org/node/2410773 ?
Comment #4
joachim CreditAttribution: joachim commentedYup, added that to the CR for the other issue.
Comment #5
joachim CreditAttribution: joachim commentedNot quite the whole story -- I forgot block module is no longer a core requirement.
Comment #6
Manuel Garcia CreditAttribution: Manuel Garcia commentedComment #7
jhodgdonThe docs seem clear to me, thanks!
Comment #8
webchickThe docs seem fine, but is it better to remove the dependency? It's certainly non-obvious that you would need that from the name.
Comment #9
joachim CreditAttribution: joachim commentedTesting that the breadcrumb works is a part of testing Field UI is working correctly.
I guess one way to lessen the possibility of WTF further is to make the assertion for the breadcrumb optional by wrapping it in a check for block module & the breadcrumb block, and tweak the documentation in this patch slightly to say it's recommended to have the breadcrumb block enabled. Then core tests that use this trait can have the breadcrumb block, and contrib modules that use this trait can do so without having to worry about having the breadcrumb.
But it does mean they won't be fully testing their use of Field UI. I suppose it probably doesn't matter though -- if the breadcrumb works for core entities, it probably works for anything contrib can throw at it?
Comment #10
alexpottLet's do #9. It seems sensible to check this before making an assertion on it.
Comment #11
kfitz CreditAttribution: kfitz at Acro Commerce commentedI've adjusted the wording in the documentation in the top of the file, and added a conditional to see if the block module and bread crumb block modules are enabled. However, I don't believe the strings I used to identify the modules are necessarily accurate, as I was not sure where to find them. Let me know how I can improve on this one (pardon the lack of interdiff, but the changes are quite minimal).
Comment #12
kfitz CreditAttribution: kfitz at Acro Commerce commentedComment #13
jhodgdonThanks for the patch!
With the code change, this is no longer just documentation, so I'm moving this to the Field UI component.
Also, a couple of things to fix in the patch:
This is fairly clear, but:
- It doesn't tell me why it's recommended. Maybe instead of saying it's recommended, it should tell me that if the breadcrumb block has been placed then breadcrumbs will be tested?
- The formatting needs to be updated. The text should all wrap to just under 80 character lines.
This is not right. system_breadcrumb_block is not a module, so the if() here will always fail and the link will never be tested.
Comment #15
kfitz CreditAttribution: kfitz at Acro Commerce commentedMade changes to documentation note and updated string in the conditional.
Comment #17
kfitz CreditAttribution: kfitz at Acro Commerce commentedTest was failing due to visibility of 'moduleExists' in FieldUiTestTrait. Switched to '\Drupal:;moduleHandler()->moduleExists', but I think there may be a bit of performance hit using this. Additionally, I've still not confirmed that 'breadcrumb' is in-fact the identifier for the Breadcrumb module, and I was not able to find the module for D8.
Comment #18
kfitz CreditAttribution: kfitz at Acro Commerce commentedComment #19
joachim CreditAttribution: joachim commentedThat's just checking the modules exist rather than that the breadcrumb block is there, no? We should be checking the block is present in the theme rather than modules.
Also, AFAICT there are no modules with either of those names. The breadcrumb block is probably provided by the system module.
Comment #21
joachim CreditAttribution: joachim commentedFixed the check for the breadcrumb test to check a) the block module is enabled and b) a breadcrumb block is present.
Also, spotted another place in the trait where breadcrumbs are tested.
Comment #23
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis was discussed during a major issue triage call and we think that it is not really major because contrib and custom tests have had more than a year to fix their tests.
However, the patch looks ready to go :)
Comment #24
alexpottShould we have a test that uses the fielduitesttrait without the block module installed in core? Then we can be sure that we don't break this in the future and know that the dependency is truly removed?
Comment #25
joachim CreditAttribution: joachim commentedI don't think this needs tests. It just needs documentation: 'this test trait requires X Y Z in order to function correctly'.
> know that the dependency is truly removed?
That's a separate problem from this issue, surely.