Problem/Motivation

The \Drupal\system\Form\ThemeAdminForm can set the admin theme to 0 in the case where we want it to be the same as the default theme. There is no 0 theme. Also many many tests are conducted without an admin setting in system.theme because it is missing from the testing profile. Furthermore the default supplied by the system module is an empty string. Therefore if you go to the themes page and press save after install minimal configuration changes!

This is a mess that is leading to bugs. For example in the quickedit module we have the following code:

    $theme = Drupal::config('system.theme')->get('admin');

    // First let the base theme modify the library, then the actual theme.
    $alter_library = function (&$library, $theme) use (&$alter_library) {
      if (isset($theme) && $theme_path = drupal_get_path('theme', $theme)) {

If the $theme is 0 or an empty string then this code emits an error but we don't find this out in our tests because they are using the testing profile. This is shown by the latest patches on #2352949: Deprecate using Classy as the default theme for the 'testing' profile

Proposed resolution

  • Make the default value for using the same theme as default and admin an empty string
  • Update system.theme:admin to empty string when it is current 0 as it is a nonsense value
  • Fix the quickedit code to do !empty($theme) - this alter hook only needs to add the admin libraries in if there is an admin theme set.
  • Improve test coverage
  • The testing profile defaults will be dealt with by #2352949: Deprecate using Classy as the default theme for the 'testing' profile

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Original report

Hi,
I've got problem with Bartik theme. The problem is resolved after using drush cache-rebuild, but after some time it is showing up again.

User warning: The following theme is missing from the file system: in drupal_get_filename() (line 232 of core/includes/bootstrap.inc).

drupal_get_filename('theme', '')
drupal_get_path('theme', '')
{closure}(Array, '')
quickedit_library_info_alter(Array, 'quickedit', NULL)
Drupal\Core\Extension\ModuleHandler->alter('library_info', Array, 'quickedit')
Drupal\Core\Asset\LibraryDiscoveryParser->parseLibraryInfo('quickedit', 'core/modules/quickedit')
Drupal\Core\Asset\LibraryDiscoveryParser->buildByExtension('quickedit')
Drupal\Core\Asset\LibraryDiscoveryCollector->getLibraryDefinitions('quickedit')
Drupal\Core\Asset\LibraryDiscoveryCollector->resolveCacheMiss('quickedit')
Drupal\Core\Cache\CacheCollector->get('quickedit')
Drupal\Core\Asset\LibraryDiscovery->getLibrariesByExtension('quickedit')
Drupal\Core\Asset\LibraryDiscovery->getLibraryByName('quickedit', 'quickedit')
Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies(Array)
Drupal\Core\Asset\LibraryDependencyResolver->getLibrariesWithDependencies(Array)
Drupal\Core\Asset\AssetResolver->getLibrariesToLoad(Object)
Drupal\Core\Asset\AssetResolver->getCssAssets(Object, 1)
Drupal\Core\Render\HtmlResponseAttachmentsProcessor->processAssetLibraries(Array, Array)
Drupal\Core\Render\HtmlResponseAttachmentsProcessor->processAttachments(Object)
Drupal\Core\EventSubscriber\HtmlResponseSubscriber->onRespond(Object, 'kernel.response', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.response', Object)
Symfony\Component\HttpKernel\HttpKernel->filterResponse(Object, Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jakow created an issue. See original summary.

emma.maria’s picture

Component: Bartik theme » quickedit.module

Thanks @jakow. I am going to assign this over to the quick edit module team to take a look. From looking at the error I have a feeling this issue could occur with any theme.

Wim Leers’s picture

Category: Bug report » Support request
Status: Active » Postponed (maintainer needs more info)
drupal_get_filename('theme', '')
drupal_get_path('theme', '')
{closure}(Array, '')
quickedit_library_info_alter(Array, 'quickedit', NULL)

This indicates that either of these return the empty string:

$theme = Drupal::config('system.theme')->get('admin');
$theme === '';

or

$info = system_get_info('theme', $theme);
$info['base theme'] === '';

Are you sure this is happening with plain Bartik? I suspect you've modified Bartik? Nobody else has reported this.

markhalliwell’s picture

Title: Problem with quick_edit module in Bartik theme » quickedit_library_info_alter doesn't handle admin theme properly
Version: 8.0.0-rc1 » 8.0.x-dev
Category: Support request » Bug report
Status: Postponed (maintainer needs more info) » Active
Related issues: +#2416673: Add a ThemeConfig service for setting and getting default and admin themes

The problem is with:

$theme = Drupal::config('system.theme')->get('admin');

If the admin theme is set to "0" (default theme), then this error occurs.

The attached related issue should help with this, but not postponing on it since this can be fixed independently (check for 0 and use default instead).

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

netw3rker’s picture

I just ran into this problem too.

This appears to occur after a config export and then a config import. (the export supplies the "0" and the import imports it as a 0 even though it is initially stored as null). Users could just go in and save the theme settings page and this would resolve itself too.

I'd say a big part of the reason nobody else has reported this is that people using CMI *and* an admin theme that is their default theme is quite low.

I've attached a patch that checks for a 0 theme, and gets the default.

Hope this helps!

Wim Leers’s picture

Title: quickedit_library_info_alter doesn't handle admin theme properly » quickedit_library_info_alter() doesn't handle admin theme properly because config export+import sets the admin theme to "0"
Component: quickedit.module » configuration system

I'd say a big part of the reason nobody else has reported this is that people using CMI *and* an admin theme that is their default theme is quite low.

That seems probable.

This appears to occur after a config export and then a config import. (the export supplies the "0" and the import imports it as a 0 even though it is initially stored as null).

Isn't this the real bug?

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

malaynayak’s picture

Updating patch #10 as it fails to apply using composer. Also adding a condition to check if the admin theme is an empty string and load the default theme.

alexpott’s picture

alexpott’s picture

Here's a fix that addresses #11 ie. the root cause of the problem.

Still need to write upgrade path.

alexpott’s picture

alexpott’s picture

Title: quickedit_library_info_alter() doesn't handle admin theme properly because config export+import sets the admin theme to "0" » Form sets system.theme:admin to '0' breaking quickedit and making no sense
Component: configuration system » theme system
Krzysztof Domański’s picture

Version: 8.6.x-dev » 8.8.x-dev

The last submitted patch, 15: 2587119-15.patch, failed testing. View results

The last submitted patch, 15: 2587119-15.test-only.patch, failed testing. View results

alexpott’s picture

Ah another bug revealed with our testing profile because the admin theme is not set we're not correctly identifying the selected theme as the admin theme in the \Drupal\Tests\system\Functional\System\ThemeTest::testInstallAndSetAsDefault() test.

Jaesin’s picture

Since `\Drupal\Core\Theme\ThemeNegotiator::determineActiveTheme()` only checks for NULL, should `\Drupal\user\Theme\AdminNegotiator::determineActiveTheme` return NULL when the admin theme is empty?

Jaesin’s picture

+++ b/core/modules/quickedit/tests/src/Functional/QuickEditMinimalTest.php
@@ -0,0 +1,50 @@
+    $node = $this->createNode(['type' => 'article']);

The $node variable is unused.

This also prevents the admin theme negotiator from returning an empty value which would result result in checking access for a non-existent theme.

alexpott’s picture

+++ b/core/modules/user/src/Theme/AdminNegotiator.php
@@ -79,7 +79,8 @@ public function applies(RouteMatchInterface $route_match) {
-    return $this->configFactory->get('system.theme')->get('admin');
+    $theme = $this->configFactory->get('system.theme')->get('admin');
+    return !empty($theme) ? $theme : NULL;

If this code in HEAD causes a problem then we'd be having this with all minimal installs. This needs further investigation and if this change is necessary it needs tests.

alexpott’s picture

So re #24 yep this is not necessary. Debugging the code reveals that we do:

      if ($negotiator->applies($route_match)) {
        $theme = $negotiator->determineActiveTheme($route_match);
        if ($theme !== NULL && $this->themeAccess->checkAccess($theme)) {
          return $theme;
        }
      }

in \Drupal\Core\Theme\ThemeNegotiator::determineActiveTheme()

So $this->themeAccess->checkAccess($theme) returns FALSE for an empty string and '0' so HEAD is not broken and the change in #23 is not necessary.

Jaesin’s picture

Status: Needs review » Reviewed & tested by the community

The "improvement" in #23 is to better comply with \Drupal\Core\Theme\ThemeNegotiatorInterface::determineActiveTheme() and to avoid the access check on an invalid theme value but I concede the change isn't strictly necessary for this issue.

Everything else in #21 looks good to me except the unused variable in QuickEditMinimalTest which may be considered a nit.

When the admin theme is set to '', theme negotiation falls back to the default theme.

alexpott’s picture

Here's a even more stripped down Quickedit + Minimal test. Plus a new patch with #23 removed. Will open a follow-up for that.

Jaesin’s picture

#27 looks good to me.

alexpott’s picture

The last submitted patch, 27: 2587119-27.test-only.patch, failed testing. View results

Wim Leers’s picture

Title: Form sets system.theme:admin to '0' breaking quickedit and making no sense » Form sets system.theme:admin to '0' breaking Quick Edit and making no sense
Priority: Normal » Critical
Issue tags: +blocker

I was going to +1 this RTBC, but then I noticed this bit from my comment from 1.5 year ago in #11:

This appears to occur after a config export and then a config import. (the export supplies the "0" and the import imports it as a 0 even though it is initially stored as null).

Isn't this the real bug?

If that was true, then this patch should contain some changes for the configuration system, or for the system.theme config in particular. But it does not.

Either that statement was wrong, or the patch is incomplete.

So I tried to manually reproduce this, but could not. Therefore the statement was wrong.

RTBC++ ✅


Finally, since this is blocking #2352949: Deprecate using Classy as the default theme for the 'testing' profile, which is critical, bumping priority and tagging.

alexpott’s picture

@Wim Leers config is fixed as part of this change. The value in system.theme:admin is updated if it is '0'

  1. +++ b/core/modules/system/src/Form/ThemeAdminForm.php
    @@ -38,7 +38,7 @@ public function buildForm(array $form, FormStateInterface $form_state, array $th
    -      '#options' => [0 => $this->t('Default theme')] + $theme_options,
    +      '#options' => ['' => $this->t('Default theme')] + $theme_options,
    

    This results in '0' no longer ending up in config.

  2. +++ b/core/modules/system/system.install
    @@ -2357,3 +2357,16 @@ function system_update_8801() {
    +function system_update_8802() {
    +  $config = Drupal::configFactory()->getEditable('system.theme');
    +  // Replace '0' with an empty string as '0' is not a valid value.
    +  if ($config->get('admin') == '0') {
    +    $config
    +      ->set('admin', '')
    +      ->save(TRUE);
    +  }
    +}
    

    And this updates all the current config if '0' is the value.

  • lauriii committed 50ac19a on 8.8.x
    Issue #2587119 by alexpott, Jaesin, malaynayak, netw3rker, Wim Leers:...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Thank you @Wim Leers for walking through this with me. It made reviewing this much faster.

  1. +++ b/core/modules/quickedit/quickedit.module
    @@ -79,7 +79,7 @@ function quickedit_library_info_alter(&$libraries, $extension) {
    -      if (isset($theme) && $theme_path = drupal_get_path('theme', $theme)) {
    +      if (!empty($theme) && $theme_path = drupal_get_path('theme', $theme)) {
    

    ✅ This change is not required but I'm +1 to making this so that Quickedit works with the '0' admin theme configuration. This is useful since there might be installation profiles with invalid default configurations.

  2. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
    @@ -458,9 +458,10 @@ public function testInstallAndSetAsDefault() {
    -      $this->assertTrue((bool) preg_match("/$theme_name " . preg_quote($version) . '\s{2,}\(default theme\)/', $out));
    +      $this->assertTrue((bool) preg_match("/$theme_name " . preg_quote($version) . '\s{2,}\(default theme, administration theme\)/', $out));
    

    ✅ Good catch! Confirmed that with the '' admin theme this was showing an incorrect list of theme notes.

Committed 50ac19a and pushed to 8.8.x. Thanks!

Wim Leers’s picture

#32: sorry for the confusion, I was just proving my past self wrong, and sharing the reasoning :)

Status: Fixed » Closed (fixed)

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