Problem/Motivation

This came up in #3259925: Use Olivero instead of Bartik as example theme in documentation, to be precise: @Berdir brought this up in https://git.drupalcode.org/project/drupal/-/merge_requests/938#note_36419

There are a bunch of tests currently that loop over "all themes" but do not include Olivero.

Steps to reproduce

-

Proposed resolution

Include Olivero in those tests.

Remaining tasks

User interface changes

-

API changes

-

Data model changes

-

Release notes snippet

CommentFileSizeAuthor
#12 325992-10.patch10.92 KBtstoeckler
#12 3259928-6-10-interdiff.txt2.49 KBtstoeckler
#6 3259928-6.patch10.19 KBtstoeckler
#6 3259928-3-6-interdiff.txt6.3 KBtstoeckler
#3 3259928-2.patch5.37 KBtstoeckler

Comments

tstoeckler created an issue. See original summary.

tstoeckler credited Berdir.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review
StatusFileSize
new 3259928-2.patch5.37 KB

Here we go.

Crediting @Berdir for the idea.

Status: Needs review » Needs work

The last submitted patch, 3: 3259928-2.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Sorry for the known-fail retest, but after #3257407: Use "content" region in BlockCreationTrait::placeBlock() instead of "sidebar_first" I think this will have a few less failures and that will save me some time when I get back to finishing this hopefully over the weekend.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 3259928-3-6-interdiff.txt6.3 KB
new 3259928-6.patch10.19 KB

OK, the re-test was less helpful than I had hoped. Anyway, unless I messed something up, this should be green.

Also see #3259928: Change various tests that test with "all themes" to also include Olivero for the todo in SettingsTrayIntegrationTest (and why this took me so long to finish...).

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Meant to link #3262273: Quickedit may attempt to initialize a field before loading its editor in the previous comment.

tstoeckler credited m4olivei.

tstoeckler credited marcoscano.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Crediting @marcoscano and @m4olivei from #3219959: Update standard profile so Olivero is the default theme as I stole most of this from there. If I have overlooked someone else please let me know!

Status: Needs review » Needs work

The last submitted patch, 6: 3259928-6.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 3259928-6-10-interdiff.txt2.49 KB
new 325992-10.patch10.92 KB

OK this one makes sense, I had both missed another usage of the sidebar_first region which Olivero doesn't have and also missed that method's test failure in the previous test result and had only checked the other failure locally. So this should be green, let's see...

Also did another self-review and added two little comments.

Note that this is now pretty close to being a duplicate of #3262320: Remove obsolete region override in ContextualLinksTest but I can't really justify bringing the hunk from ContextualLinksTest over here as that just tests with Stark, so leaving as is and either one will have to be rebased/rerolled when the other is committed, but that shouldn't be a big issue.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

I think this patch looks good given all the constraints it needed to work with. Sad to need to have all the @todos but they are unrelated bugfixes.

  • catch committed c40c2c9 on 10.0.x 
    Issue #3259928 by tstoeckler, Berdir, m4olivei, marcoscano, Gábor Hojtsy...
  • catch committed 91947b2 on 9.4.x
    Issue #3259928 by tstoeckler, Berdir, m4olivei, marcoscano, Gábor Hojtsy...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

The @todos are mostly going to be resolved by issue that this was split out from, so not a lot we can do about adding them, at least there's a clear path to removing those.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Awesome, thank you!

That now reduces the scope of #3262320: Remove obsolete region override in ContextualLinksTest to just one test (so re-titled accordingly), so hopefully that will be easy to review and get in, as well.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.