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.Sorry for having to start this discussion again... Sadly, it's not working right!
The problems it runs into are the same we have with drupal_alter() and the fact that themes aren't modules. Let me explain... Whenever we are on the backend (even when we are on the theme settings page of a particular theme) we will not invoke the alter hooks for that theme. Neither will we invoke the libraries info hook for themes as introduced by this patch.
This is because a) global $theme is always the theme we are actually rendering (not the theme we are displaying the theme settings for... there are actually other related issues/bugs in Core already that show this pretty well... anyways...) and b) even if we switched to global $theme_key it would not fix it because whenever we build the cache for the libraries and happen to be in the backend (other than the theme settings page) it will still ignore the front-end theme.
So... What we really need is for this lookup to be completely agnostic of the active theme. We have to loop over all themes and their base themes regardless of whether they are active or not. Also, we should add a $type key to the info array somewhere so we know whether the origin is a module or a theme... Or something else that gives us that particular information.
I am filing this as a bug report because even though it doesn't break anything it simply doesn't work the way it was supposed to work.
Patch incoming...
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | theme-integration-files.patch | 8.27 KB | tstoeckler |
| #10 | 1876124-libraries-themes-10.patch | 6.1 KB | tstoeckler |
| #15 | themes-rename-everything.patch | 47.79 KB | tstoeckler |
| #16 | theme-test.patch | 5.15 KB | tstoeckler |
Comments
Comment #1
mmikitka CreditAttribution: mmikitka commentedHi fubhy,
I am also interested in a patch to this issue, and I'm available to help. I'm new to Drupal dev, but am willing to learn the ropes.
Let me know if help is needed.
matt
Comment #2
barraponto CreditAttribution: barraponto commentedI'm unsure as to why would a library declared in a theme be loaded when that theme is not the active theme. If you need the library in the admin theme, then declare it in the admin theme as well.
Or if you want the library to be available across several themes, declare it in a module.
Comment #3
fubhy CreditAttribution: fubhy commentedIt has nothing to do with that. I don't want to use it with disabled or inactive themes, you missed my point ;). The problem is this: If you have an admin theme on your page and you clear the cache while in the backend it will then, when reloading the page, rebuild the cache (module cache + admin theme, because the admin theme is active at that point). But... by doing so, populates the cache with the libraries declared in the admin theme. Now, when you leave the backend, the frontend theme will not be able to load it's libraries because they simply aren't in the cache.
Comment #4
fubhy CreditAttribution: fubhy commentedOr, think about the themekey or switchtheme modules. Their libraries won't, ever, get their libraries registered. Unless, of course, the cache is empty in which case, however, the other theme won't get it's libraries registered. Hence, we need to either split up the libraries registry and provide both, a theme libraries registry and a module libraries registry (in separate) and merge them when loading the page or cache the entire registry together but by adding the theme key of the active theme to the cache key.
Comment #5
barraponto CreditAttribution: barraponto commentedWhat happens if a theme declares its library in
hook_libraries_info_alter? It's been used that way in Kalatheme for a while with no issues AFAIK.If it works with _alter, maybe we should just document that.
I'm not really convinced themes should declare libraries at all, but that's not my call to make.
Comment #6
tstoecklerYeah, this is a pretty big problem. Working on it.
Comment #7
tstoecklerHow about this? This is completely untested.
Speaking of which, this needs (automated) tests.
Comment #8
tstoecklerRe #5 the alter hook suffers from the same problem that was described here. The patch in #7 should solve that problem as well.
Comment #10
tstoecklerThis at least fixes the existing tests, I hope.
Comment #11
whastings CreditAttribution: whastings commentedI looked into this issue while working on a symptom of it in Kalatheme (see #2120641: Fix Bootstrap library detection failure). The patch from #10 fixes our issue and seems to be in working order.
Comment #12
tstoecklerThanks @whastings for testing this patch! I will commit this soon and work on tests in a follow-up.
Comment #13
nobodypanic CreditAttribution: nobodypanic commentedsorry, it was a long weekend, figured out my issue with applying the patch.
Comment #14
pirog CreditAttribution: pirog commented+1 on @whastings #11.
This seems to resolve the issue at #2120641: Fix Bootstrap library detection failure at least.
Comment #15
tstoecklerCommitted #10 to 7.x-2.x and 7.x-3.x.
The following patch does not yet provide any tests but in order to provide a test theme we need to move a bunch of stuff around and rename stuff, with the attached patch does.
The amount of changes that are necessary is proof that our test suite is simply unmaintainable, but that's for another day...
Comment #16
tstoecklerCommitted that to 7.x-2.x and 7.x-3.x as well.
Attached patch contains some tests. Will commit that as well, if it's green.
I noticed, however, that themes currently cannot provide integration files, that is hardcoded to modules. So that's one other thing we'll have to fix in this issue.
Comment #17
tstoecklerCommitted that to 7.x-2.x and 7.x-3.x, as well.
This patch fixes integration files for themes. This should be the last one.
Comment #18
tstoecklerAlright committed that as well (7.x-2.x, 7.x-3.x).
I'd like for some more people to test this in the wild and then release a new version pretty soon. Since we announced theme support with the last version, but it's not really working, we should get this out soon.
Comment #19
tstoecklerComment #20
pirog CreditAttribution: pirog commentedWe've gotten three or four GTGs on this as far as Kalatheme goes.
Comment #21
tstoecklerAwesome thanks for the update. There's one BC-break that we have currently relative to 7.x-2.1, which is very minor, but which I've sort of come around trying to avoid. So it might still be a week or two until the next release. I hope it's going to be earlier, just wanted to give a heads up.
Comment #23
kaidjohnson CreditAttribution: kaidjohnson commentedIn v2.1, we were able to declare hook_libraries_info() in our base theme and use it with libraries_detect/libraries_load in our subthemes, even when the base theme is 'disabled.' In v2.2, our base theme must now be 'enabled' in order for its hook_libraries_info() declaration to be made available to its subtheme. It's not a huge issue, but it is certainly worth noting for folks using a similar structure.
Comment #24
rokat CreditAttribution: rokat commentedAs for me the issue is quite random, it appeared on some deployment servers and not during production.
Anyway enabling the "parent theme" did do the job for me.
Thanks for the tip
Comment #25
plachRelated issue: #2815965: Base theme is not loaded when checking for theme library info