If you install with the Umami demo profile and go to the Appearance page, the Umami theme isn't listed there. This means that if you switch to a different theme, you can never switch back.

This can be fixed simply by removing the hidden: true from the theme's info file. My understanding is that originally existed to prevent users of other install profiles from seeing the theme, but it isn't necessary now that the theme lives inside the profile (since putting it inside the profile in the codebase automatically prevents sites installed with a different profile from seeing it).

This was intended to be fixed in #2809635-171: Create experimental installation profile, but somehow that code change didn't make it into the final commit on that issue?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
458 bytes

Here is a patch.

Berdir’s picture

Ha, was wondering about this one too.

You also had the same thought as I did.. "Hm, this looks nice, lets see what happens if I switch to Bartik... oops." :)

smaz’s picture

Status: Needs review » Reviewed & tested by the community

Apologies, I thought that fix was included in one of the last patches for the main issue - must have gone AWOL at some point.

I've already tested this change before, so marking as RTBC:
https://github.com/lauriii/umami/commit/4e382910b956573c1e93ee463572e23d...

markconroy’s picture

In my case, I switched to Bartik when on simplytest me testing the layout builder modules (so I'd have some content to see/work with), then couldn't get back to my beloved Umami.

Nice to see it fixed.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Apologies, I thought that fix was included in one of the last patches for the main issue - must have gone AWOL at some point.

Can we expand on the existing tests to visit the admin appearance page and assert the umami theme is present.

Slowly...expanding...the test coverage...one...issue...at...a...time

Thanks folks

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
890 bytes
1.32 KB

Something like this?

larowlan’s picture

+++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
@@ -110,4 +110,16 @@ public function testEditNodesByAdmin() {
+    $webassert->pageTextMatches('/Umami\s+\(default theme\)/');

Any reason not to use pageTextContains?

David_Rothstein’s picture

So with the theme displaying on the Appearance page again, that actually exposes a couple other bugs - adding those as related issues now.

David_Rothstein’s picture

Any reason not to use pageTextContains?

Not sure - I was basically just roughly copying this from https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Tests%... (which uses a regular expression also).

The last submitted patch, 7: 2938918-7-TESTS-ONLY.patch, failed testing. View results

markconroy’s picture

New patch using pageTextContains

(This'll be my first ever commit in a test if it's commited!)

navneet0693’s picture

Status: Needs review » Reviewed & tested by the community

That should be all good.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6e06ffe and pushed to 8.6.x. Thanks!

  • catch committed 6e06ffe on 8.6.x
    Issue #2938918 by David_Rothstein, markconroy, larowlan: Impossible to...
smaz’s picture

Status: Fixed » Needs review

Setting to needs review just to check something...

I think that we actually need the first patch in #7, and not the one from #12.

The test is for checking that Umami is the default theme ("Tests that the Umami theme is the default theme on the Appearance page."), but using pageTextContains("Umami") just checks that the theme is there, not enabled as default.

The regex in pageTextMatches() is used instead of pageTextContains(), due to the version number of the theme being between the theme name & (default theme) text:
Umami theme in the appearance page

markconroy’s picture

@smaz,

I don't think we need to test if the theme is the default theme or not, just to be able to switch back to it. All we need to do is make sure that the theme is available on the page.

Maybe we could do another test to check if it is the default theme as well (but that would should probably be run at install time - once the site is installed, check that Umami is the default theme).

catch’s picture

Status: Needs review » Fixed

Agreed with #17, however the comment for the test does say it's checking for the default theme, not that the theme is available, which is the bug this patch is fixing. @smaz could you open a follow-up to expand the test coverage and/or fix that comment?

David_Rothstein’s picture

  • alexpott committed bbb1757 on 8.5.x authored by catch
    Issue #2938918 by David_Rothstein, markconroy, larowlan: Impossible to...

Status: Fixed » Closed (fixed)

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