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)
Comment | File | Size | Author |
---|---|---|---|
#27 | 2587119-27.patch | 8.22 KB | alexpott |
#27 | 2587119-27.test-only.patch | 1.12 KB | alexpott |
#27 | 23-27-interdiff.txt | 1.67 KB | alexpott |
#23 | 2587119-23.patch | 9.12 KB | Jaesin |
#23 | 21-23-interdiff.txt | 1.27 KB | Jaesin |
Comments
Comment #2
emma.mariaThanks @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.
Comment #3
Wim LeersThis indicates that either of these return the empty string:
or
Are you sure this is happening with plain Bartik? I suspect you've modified Bartik? Nobody else has reported this.
Comment #4
markhalliwellThe problem is with:
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).
Comment #10
netw3rker CreditAttribution: netw3rker commentedI 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!
Comment #11
Wim LeersThat seems probable.
Isn't this the real bug?
Comment #13
malaynayak CreditAttribution: malaynayak as a volunteer and at TA Digital commentedUpdating 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.
Comment #14
alexpottComment #15
alexpottHere's a fix that addresses #11 ie. the root cause of the problem.
Still need to write upgrade path.
Comment #16
alexpottHere's an upgrade path.
Comment #17
alexpottComment #18
Krzysztof DomańskiComment #21
alexpottAh 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.
Comment #22
Jaesin CreditAttribution: Jaesin at Chapter Three commentedSince `\Drupal\Core\Theme\ThemeNegotiator::determineActiveTheme()` only checks for NULL, should `\Drupal\user\Theme\AdminNegotiator::determineActiveTheme` return NULL when the admin theme is empty?
Comment #23
Jaesin CreditAttribution: Jaesin at Chapter Three commentedThe
$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.
Comment #24
alexpottIf 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.
Comment #25
alexpottSo re #24 yep this is not necessary. Debugging the code reveals that we do:
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.Comment #26
Jaesin CreditAttribution: Jaesin at Chapter Three commentedThe "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.
Comment #27
alexpottHere's a even more stripped down Quickedit + Minimal test. Plus a new patch with #23 removed. Will open a follow-up for that.
Comment #28
Jaesin CreditAttribution: Jaesin at Chapter Three commented#27 looks good to me.
Comment #29
alexpottOpened #3084078: AdminNegotiator::determineActiveTheme() does not adhere to the interface to address #22 and on
Comment #31
Wim LeersI was going to +1 this RTBC, but then I noticed this bit from my comment from 1.5 year ago in #11:
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.
Comment #32
alexpott@Wim Leers config is fixed as part of this change. The value in
system.theme:admin
is updated if it is'0'
This results in
'0'
no longer ending up in config.And this updates all the current config if
'0'
is the value.Comment #34
lauriiiThank you @Wim Leers for walking through this with me. It made reviewing this much faster.
✅ 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.✅ 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!
Comment #35
Wim Leers#32: sorry for the confusion, I was just proving my past self wrong, and sharing the reasoning :)