I can not uninstall Classy theme

Yhea, this is crtiical.

I'm not sure this is critical. The exception should be handled much better, sure, but if you have Bartik (or Seven) installed you cannot uninstall Classy because they depend on Classy.

Maybe the UI should disable the uninstall button for base themes when the sub-theme has been installed. Just like in the modules admin page.

It doesn't actually depend on it like modules do, as far as I know. So we just have to have a confirm button of - would you like to disable the base theme clasy too. What we do on the modules page is wrong!

Catch told me its critical. If you click a button, in a primary interface of Drupal and it trows a red flaming error - you have a critical bug.

Just a quick link for everyone looking at this issue: https://www.drupal.org/node/474684. Snippet from code:

// Base themes cannot be uninstalled if sub themes are installed, and if
      // they are not uninstalled at the same time.
      // @todo https://www.drupal.org/node/474684 and
      //   https://www.drupal.org/node/1297856 themes should leverage the module
      //   dependency system.
      if (!empty($list[$key]->sub_themes)) {
        foreach ($list[$key]->sub_themes as $sub_key => $sub_label) {
          if (isset($list[$sub_key]) && !in_array($sub_key, $theme_list, TRUE)) {
            throw new \InvalidArgumentException("The base theme $key cannot be uninstalled, because theme $sub_key depends on it.");
          }
        }
      }

If you click a button, in a primary interface of Drupal and it trows a red flaming error - you have a critical bug.

Good point :)

If the base theme of an admin theme can't be uninstalled, can't we remove the ability to install a theme if it's a base theme of an admin theme. Something like adding a conditional to the themepages method.

this is the code at the moment.

if (!empty($theme->status)) {
          if (!$theme->is_default) {
            if ($theme->getName() != $admin_theme) {
              $theme->operations[] = array(
                'title' => $this->t('Uninstall'),
                'route_name' => 'system.theme_uninstall',
                'query' => $query,
                'attributes' => array('title' => $this->t('Uninstall !theme theme', array('!theme' => $theme->info['name']))),
              );
            }

could change

if ($theme->getName() != $admin_theme) {

to

if ($theme->getName() != $admin_theme && !array_key_exists($admin_theme, $theme->sub_themes)) {

I think this is the right way to go, i've added in a check to see on the admin page to check if the theme is either the base theme of an admin theme or of the default theme and if so the uninstall isn't displayed.

Reviewed the patch and tested on a fresh install.
Looks really good: the link to uninstall the base theme ("Classy") is gone and you cannot uninstall the theme anymore as long as any dependent theme is installed.

waiting for the testbot before setting to RTBC ...

@ACF: could you reroll the test-patch? It seems to fail (guess the matching string is somehow not available).

@stBorchert The test patch is the test only without the change so it should fail, sorry it should be labelled test-only.

Ah. Well then https://www.drupal.org/files/issues/drupal8-2348793-11.patch is RTBC. Thanks for the patch and the clarification :)

Can we improve the the test so that we test that after uninstalling Bartik we can uninstall Classy. At the moment the test is very susceptible to false positives.

What we should do is a confirm form, if you click on uninstalling Bartik you get a confirmation form to also disable the other themes. Why cant we do this?

@Bojhan, well if you are using Seven it would also require uninstalling Seven to uninstall Classy, so clicking uninstall on Bartik and confirming would remove Bartik, Seven, and Classy. Can we maybe discuss it more in a non-critical follow-up issue though?

I think this needs more work beyond test coverage because it's still pretty breakable:

1. Clean install of D8 using the Standard profile
2. On /admin/appearance click 'Install and set default' for Stark
3. Change the administration theme to 'Default theme' and Save configuration
4. Now the Classy 'Uninstall' link is available and clicking it results in an InvalidArgumentException

Yeah, agreeing with Cottser, this is a base theme dependency issue. Uninstalling all the other themes and base themes because you want to uninstall one is excessive. Just removing the uninstall link next to the base theme makes sense.

Yep it needs to check for whether those themes are installed. Just working on better test coverage and will change the fix.

I am not really keen on doing "easy" fixes over "simple" fixes at this point in the release.

Because this simply introduces the issue, that people have no clue how to disable classy. By doing it as I propose you are aware of the affect. I am working on another issue, that we need to provide a separate style for base themes on the appearance page.

Overrall, I think I agree that this is probably not the ideal fix, but i've added a fix and some updated test coverage in the hope it's of some use for a better fix.

I'm not seeing uninstalling all the other themes as something simple. That could have rather dramatic affects. Would it be reasonable to add text with the themes stating their dependencies and requirements, like we do with modules?

Requires: X
Required by: Y

Maybe there's some misunderstanding, so here's an attempt at thinking about ways we can always keep the Uninstall links.

This makes some sense to me:

Click Uninstall on Classy, be asked whether you want to uninstall Bartik, Seven, and any other installed themes that depend on Classy (because they need to be uninstalled first before you can uninstall Classy).

This doesn't:

Click Uninstall on Bartik, be asked whether you want to uninstall Bartik, Seven, Classy, and any other installed themes that depend on Classy (just because I want to uninstall Bartik doesn't mean I want to uninstall Classy, maybe I'm just switching to another base theme that extends from Classy).

Edit: And yes what @davidhernandez said seems like it could help clarify this to the end user.

Sending #26 to testbot.

Click Uninstall on Classy, be asked whether you want to uninstall Bartik, Seven, and any other installed themes that depend on Classy (because they need to be uninstalled first before you can uninstall Classy).

I would not expect this behaviour given how admin/modules/uninstallworks at the moment.

Can't we just tell people why they cannot uninstall a specific theme?

In my mind, the uninstall link would be styled as a disabled danger link from the style guide - https://groups.drupal.org/node/283223#Buttons

We could then add a tooltip that explains the reason it is disabled on hover.

this confused me too ...
but why an extra base theme? is this temporary?
otherwise it's totally not understandable to start a project like d8 with a transparent "helper" theme ...

Re-roll of https://www.drupal.org/files/issues/drupal8-2348793-25.patch

Thanks for the re-roll. Patch works as expected, the link to uninstall is gone.

Committed f46096a and pushed to 8.0.x. Thanks!

Just wanted to mention that nowhere in the UI does it give an indication that a theme is using another as it's base, or that this would mean it's uninstallable.

Would be nice if there was some text like "theme X is using the base theme Z", from this the user could infer that the base theme would be uninstallable.

Or "theme Z is uninstallable because it's being used by themes X and Y" on the base theme itself.

I believe that is being actively worked on here - #2372183: Display same info for themes as for modules

I know this is closed but I had this issue, I could not install with drush and there was no UI to uninstall these. So I just copied the URL hovering over the uninstall bartik button:

https://invoice-bhs.blah.blah/admin/appearance/uninstall?theme=bartik&to...

and changed it to classy

https://invoice-bhs.blah.blah/admin/appearance/uninstall?theme=classy&to...

And it uninstalled fine.