Closed (fixed)
Project:
Drupal core
Version:
9.4.x-dev
Component:
block.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
7 Jan 2022 at 11:50 UTC
Updated:
17 Feb 2022 at 17:14 UTC
Jump to comment: Most recent
Problem/Motivation
This came up in #3219959: Update standard profile so Olivero is the default theme.
BlockCreationTrait::placeBlock() places blocks in a sidebar_first region by default. Not all themes will have such a region, however. In particular, Olivero doesn't. This means that tests using Olivero need to explicitly specify a region when using BlockCreationTrait::placeBlock() and tests that use multiple themes need to apply some logic to use the correct region depending on the theme or explicitly specify a region that is present in all themes.
Steps to reproduce
Use BlockCreationTrait::placeBlock() (also known as $this->drupalPlaceBlock() in functional tests) in a test that sets $defaultTheme = 'olivero';.
Proposed resolution
Use the content region instead. Since it doesn't actually matter where things appear in tests only that they appear, this should hopefully cause no disruption. Although it is possible that contrib tests explicitly check that something is within a layout-sidebar-first class, for example, this is not done anywhere in core, so it's not particularly likely (and also easy to fix if so).
Remaining tasks
User interface changes
-
API changes
-
Data model changes
BlockCreationTrait::placeBlock() places blocks in the content region by default instead of sidebar_first.
Release notes snippet
Issue fork drupal-3257407
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
tstoecklerHmm... that's unfortunate. Apparently that change is more disruptive than I thought. So it probably makes more sense to just use "sidebar" for Olivero and keep "sidebar_first" for the rest.
Still want to go through the rest of the test failures, though, to see what's up because the very first one already revealed a legitimate bug: #3257504: Empty toolbar tray displays a stray orientation toggle What's happening there is that the contextual links that should be clicked are now - because they are in the content region and not in the left sidebar - directly above or very close to the "stray" toolbar orientation toggle described in that issue, so Chromedriver can't properly click the contextual link. With the patch there applied that test is green again.
Comment #4
tstoecklerComment #5
tstoecklerOK, changed my mind a bit again. So all in all we have 10 failures (DrupalCI double counts some of them), 3 of which are actually #3257504: Empty toolbar tray displays a stray orientation toggle, 4 implicitly rely on the order of things in the DOM which happens to change with this and only 3 actually have assertions for "sidebar-left" markup/output.
So I worked around the first group by still having them place blocks in the left sidebar explicitly with a todo for #3257504: Empty toolbar tray displays a stray orientation toggle.
The second group I fixed by just adapting the assertions for the changed markup. 1 of these 4 I was able to fix in a way that makes it independent of the DOM ordering, but the other 3 are problems with
UiHelperTrait::clickLink()and the fact that if there are multiple identical links on a page ("Edit" in this case), the only way to target the correct one is to specify the index at which it appears in the DOM. I don't think there really is a great way to fix this "properly" i.e. make these tests more resilient to DOM changes, so I just went ahead and updated the indices.For the third group I opted for explicitly having them place blocks in the left sidebar as well, as I found it more explicit that way. The other options would have been to adapt the assertions to match the content region.
I ran all of the failed tests locally, so unless I messed something up this should be green.
Comment #6
catchGood clean-up to make test coverage more generic, and also unblocks making Olivero the default.
Comment #9
gábor hojtsyWoot, landed this in 10.x and 9.4.x. Thanks @tstoeckler!
Comment #10
tstoecklerWow, thanks for the quick commit, that's great. Filed #3262320: Remove obsolete region override in ContextualLinksTest as quick follow-up.