"Install and set as default" link on the Appearance page doesn't work

Confirmed on commit 8f9efdedddd5864b10ae7189b9a6f9296f1791eb

The error came from installation step. When you click install you get this error. (but theme installs) Clicking enables works without issues.

Thanks @honzakuchar. Setting to needs review so it goes through the automated tests (aka testbot). And tagging because this needs test coverage.

I'm not sure if it really needs test coverage. (writing new test for this) The code I've updated will be deleted in Drupal 9.0.

Where should go regression tests? Just into ThemeHandlerTest.php ?

@Cottser bump

Yup, what David said. Thanks :)

Thank you @David_Rothstein, I got a little idea based on your suggestion. just creating a tests for "Install and set a default" link. Please let me know if I should not create a new testfunctions. I will optimize the code once I get more clarification on this.

Added code looks like:

/**
   * Test installing and switching to default theme.
   */
  function testInstallAndSwitchDefaultTheme() {
    // Install Bartik and set it as the default theme.
    \Drupal::service('theme_handler')->install(array('bartik'));
    $this->drupalGet('admin/appearance');
    $this->clickLink(t('Set as default'));
    $this->assertEqual($this->config('system.theme')->get('default'), 'bartik');

    // Test the default theme on the secondary links (blocks admin page).
    $this->drupalGet('admin/structure/block');
    $this->assertText('Stark(' . t('active tab') . ')', 'Default local task on blocks admin page is the default theme.');
    
    // Switch back to Stark and test again to test that the menu cache is cleared.
    $this->drupalGet('admin/appearance');
    // Classy is the first 'Set as default' link.
    $this->clickLink(t('Install and set as default'), 0);
    $this->drupalGet('admin/structure/block');
    $this->assertText('Stark(' . t('active tab') . ')', 'Default local task on blocks admin page has changed.');
  }

Logically I should not install bartik on this tests. i am already on admin/appearance page.

Seems some API also has been changed. So adding needs reroll

error: patch failed: core/modules/system/src/Tests/System/ThemeTest.php:254
error: core/modules/system/src/Tests/System/ThemeTest.php: patch does not apply

As per #7, removed return from uninstall method, and added simpletest method to existing ThemeTest class.

I have test this patch out #13 and seem to fix this issue.

A few nits, sorry.

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -167,7 +167,7 @@ public function setDefault($name) {
   -    \Drupal::service('theme_installer')->install($theme_list, $install_dependencies);
   +    return \Drupal::service('theme_installer')->install($theme_list, $install_dependencies);
   

   HAH!

2. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -334,4 +334,21 @@ function testUninstallingThemes() {
   +    // Bartik comes uninstalled and has the second "Install and set as default" link.
   

   80 cols.

3. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -334,4 +334,21 @@ function testUninstallingThemes() {
   +  }
    }
   

   Needs a newline in between.

Thanks @Wim Leers.

Addressed #2 and #3. Was #1 a "that was an easy fix" HAH or a "mesch, you did something silly" HAH? :)

Yeah I'm pretty sure it's "HAH that was an easy fix" ;)

Couple more remaining nits:

1. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -334,4 +334,23 @@ function testUninstallingThemes() {
   +  /**
   +   * Test installing a theme and setting it as default.
   +   */
   +  function testInstallAndSetAsDefault() {
   

   Docblock usually start plural "Tests" per https://www.drupal.org/node/1354#functions. (I'm not an English major but this part - third person singular present tense verb).

2. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -334,4 +334,23 @@ function testUninstallingThemes() {
   +  ¶
   

   Trailing whitespace here.

Indeed it was the positive one :)

Here is the patch with #16 suggestions.

Changes look good, thanks @darol100, next time please provide an interdiff :)

I'm not sure that we've seen a test-only patch yet.

Can we get a test-only version of the patch posted, along with the same patch from #18? Like on the bottom of https://www.drupal.org/contributor-tasks/write-tests.

Per #20.

Excellent, that newline should be there :)

RTBC*, has tests and fixes the bug.

This should get kicked back to needs work because the test-only patch was uploaded second (upload it first to prevent this in the future). I do declare that anyone can re-RTBC it today on my behalf.

* The RTBC is also conditional on the test-only patch failing :)

Seems not worth retesting at this point, testbot is having a bad day.

Taking a chance, things seem to have calmed down somewhat.

Now as long as the testbot re-testing RTBC patches doesn't mess this up…

I think the simplest way to fix that is just to change it

Yep.

Addressing the fixes in #31

Fixing up!

+++ b/core/modules/system/src/Tests/System/ThemeTest.php
@@ -334,4 +334,23 @@ function testUninstallingThemes() {
+    $this->assertText('Bartik(' . t('active tab') . ')', 'Default local task on blocks admin page is the default theme.');

Would the same comment apply here?

I find the simpletest documentation on t() is a bit unclear on when and where the strings are translated. The t() function is used elsewhere in this class (e.g. testInvalidTheme()) with assertText() and likewise in the simpletest examples module.

The alternative:

$this->assertText(t('@theme is now the default theme.', array('@theme' => 'Bartik')));

But it should probably match the current translatable string in that case, which uses % and then you'd use assertRaw() instead I think. Not sure.

I do not see the value of loading the blocks admin page to check the default theme. This checks the appearance form output and for the regression caused by ThemeHandler::install() not returning anything. I am a bit of a mad wizard with regexes so somebody give it a hard look please.

This is less fragile.

Had this same issue, was looking to file a bug, then saw this issue. So I've done a manual test of patch #44. The patch fixes the issue for me and the test is great.

Manual Test ok.

RTBC++

Manually tested patch #44 and works perfectly.

RTBC from me too.

This fixes the bug and has a test - nice. Committed cffc13a and pushed to 8.0.x. Thanks!