Message	Group	Filename	Line	Function	Status
Bartik theme disabled during import.	Other	ConfigImportUITest.php	214	Drupal\config\Tests\ConfigImportUITest->testImport()	
[08-May-2014 11:25:51 UTC] Uncaught PHP Exception InvalidArgumentException: "Unknown theme: bartik." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Extension/ThemeHandler.php line 282	Fatal error	Unknown	0	Unknown	

Occasionally ConfigImportUITest fails with the above errors.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
2.53 KB
645 bytes

Maybe we're using the list before it has been initialised. Let's make the code a bit safer.

alexpott’s picture

The patch includes work from #2226301: run-test.sh --repeat should work nicely with --concurrency to allow concurrent testing of the same test class.

The last submitted patch, 1: random-config-ui-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: random-config-ui-fail.will-fail.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
alexpott’s picture

FileSize
1.9 KB

Now with just the patch

Xano’s picture

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -133,10 +133,8 @@ public function getDefault() {
       throw new \InvalidArgumentException("$name theme is not enabled.");

This seems a little out of scope, but it's not a bad change.

If nobody minds the scope creep, then this looks RTBC if the tests pass.

sun’s picture

@Xano: That line is not changed by the patch? :-)

I'm tempted to RTBC, although I wondered whether this regression could be tested in some way... e.g., a single method in ThemeHandlerTest that invokes all methods without calling listInfo() upfront, whereas the result of the methods is irrelevant, just no unexpected error. Such a test should fail in HEAD, but not with this patch?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -133,10 +133,8 @@ public function getDefault() {
    -    if (!isset($this->list)) {
    -      $this->listInfo();
    -    }
    ...
    +    $list = $this->listInfo();
    

    For the record, this is technically correct, and works fine, but the change is a good one, we shouldn't be relying on $this->list directly.
    Because...

  2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -275,10 +273,11 @@ public function enable(array $theme_list, $enable_dependencies = TRUE) {
    +    $list = $this->listInfo();
         $theme_config = $this->configFactory->get('system.theme');
     
         foreach ($theme_list as $key) {
    -      if (!isset($this->list[$key])) {
    

    It leads to this assumption, which caused the bug. $this->list can be not populated.

Great find @alexpott, thanks!

Xano’s picture

@Xano: That line is not changed by the patch? :-)

I blame Dreditor...

Just for the record: I'm fine with fixing this everywhere in the theme handler, but I just wanted to point out that only the last hunk of the patch contains the fix and the rest is just a clean-up, but a good one.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This is a good change that makes the code more robust. Committed to 8.x. Thanks.

  • Commit 77f8dc5 on 8.x by Dries:
    Issue #2263201 by alexpott: ConfigImportUITest has a random failure.
    

Status: Fixed » Closed (fixed)

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