This seems to be a regression of #474684: Allow themes to declare dependencies on modules. The code there has been unchanged since a long time, but now themes have dependencies too and if they depend on another theme without a version (apparently themes don't get a default version key), e.g. when you have a custom theme depending on another custom theme, then you get this php warning:
Notice: Undefined index: version in system_requirements() (line 909 of core/modules/system/system.install).
\Drupal\Core\Extension\ModuleExtensionList defines an empty version key as default. The easiest fix is probably to add the same to ThemeExtensionList?
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | theme-version-3133604-6.patch | 1.25 KB | dww |
| #6 | theme-version-3133604-6.patch | 1.25 KB | berdir |
| #6 | theme-version-3133604-6-tests-only.patch | 739 bytes | berdir |
| #2 | theme-version-3133604-2.patch | 542 bytes | berdir |
Comments
Comment #2
berdirComment #3
dwwLooks like a good fix. However, I think we'll need a test that tickles the bug before anyone will commit this to core.
Comment #4
tedbow@Berdir thanks for creating this issue
I found this other bug while reviewing this issue
#3133657: Themes are allowed in 'dependencies' for other themes
Comment #5
berdirWill try to work on a test, not sure how easily that's possible in core as some default version logic might be applied to anything that core provides?
Comment #6
berdirThis adds an explicit test for the ThemeExtensionList service that the version key is set, good enough? Actually testing the system requirements is awkward, doesn't really work in a kernel test it seems. Used an existing theme without version key, apparently we have a few of those.
Comment #8
dww@Berdir Looks great to me. Test-only failed thusly:
That matches the problem described in the summary. Agreed that a more elaborate test for system requirements isn't needed. The fix is incredibly small and trivial. No CS problems.
RTBC!
Thanks,
-Derek
Comment #9
dwwp.s. Not sure why the bot is struggling with the 8.9.x testing of this. Trying it locally:
a) Patch applies cleanly to a clean 8.9.x git checkout.
b) core/tests/Drupal/KernelTests/Core/Extension/ThemeExtensionListTest.php passes.
c) Reverting the fix to core/lib/Drupal/Core/Extension/ThemeExtensionList.php and the test fails as expected.
/shrug
Comment #10
dwwTestbot seems to be all kinds of confused. Re-uploading the real patch from #6 in the hopes that this will become the re-tested patch and that it'll actually work on 8.9.x. 🤞
Comment #16
xjmOops. I compared
ModuleExtensionListandThemeExtensionListand confirmed this does seem to be the correct fix.Committed to 9.1.x, and cherry-picked to 9.0.x and 8.9.x as a beta-eligible bugfix. I realized after I committed it that #474684: Allow themes to declare dependencies on modules only landed in 8.9, so there will also be a revert in the messages there on 8.8.x (it'd probably be harmless, but just in case someone was for some reason doing something based on whether the version was set...)
Thanks!