Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
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).
Proposed resolution
\Drupal\Core\Extension\ModuleExtensionList defines an empty version key as default. The easiest fix is probably to add the same to ThemeExtensionList?
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| 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!