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...

#17 theme-integration-files.patch8.27 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 149 pass(es). View
#10 1876124-libraries-themes-10.patch6.1 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 135 pass(es). View
#15 themes-rename-everything.patch47.79 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 136 pass(es). View
#16 theme-test.patch5.15 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 144 pass(es). View


mmikitka’s picture

Hi 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.


barraponto’s picture

Status: Active » Closed (works as designed)

I'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.

fubhy’s picture

Status: Closed (works as designed) » Active

It 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.

fubhy’s picture

Or, 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.

barraponto’s picture

What 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.

tstoeckler’s picture

Priority: Normal » Major

Yeah, this is a pretty big problem. Working on it.

tstoeckler’s picture

Status: Active » Needs review
Issue tags: +Needs tests
4.89 KB
FAILED: [[SimpleTest]]: [MySQL] 129 pass(es), 6 fail(s), and 0 exception(s). View

How about this? This is completely untested.

Speaking of which, this needs (automated) tests.

tstoeckler’s picture

Re #5 the alter hook suffers from the same problem that was described here. The patch in #7 should solve that problem as well.

Status: Needs review » Needs work

The last submitted patch, libraries-themes-for-real.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
1.21 KB
6.1 KB
PASSED: [[SimpleTest]]: [MySQL] 135 pass(es). View

This at least fixes the existing tests, I hope.

whastings’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I 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.

tstoeckler’s picture

Thanks @whastings for testing this patch! I will commit this soon and work on tests in a follow-up.

nobodypanic’s picture

sorry, it was a long weekend, figured out my issue with applying the patch.

pirog’s picture

+1 on @whastings #11.

This seems to resolve the issue at #2120641: Fix Bootstrap library detection failure at least.

tstoeckler’s picture

Assigned: fubhy » Unassigned
Status: Reviewed & tested by the community » Needs review
47.79 KB
PASSED: [[SimpleTest]]: [MySQL] 136 pass(es). View

Committed #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...

tstoeckler’s picture

5.15 KB
PASSED: [[SimpleTest]]: [MySQL] 144 pass(es). View

Committed 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.

tstoeckler’s picture

8.27 KB
PASSED: [[SimpleTest]]: [MySQL] 149 pass(es). View

Committed 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.

tstoeckler’s picture

Status: Needs review » Fixed

Alright 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.

tstoeckler’s picture

Issue tags: -Needs tests
pirog’s picture

We've gotten three or four GTGs on this as far as Kalatheme goes.

tstoeckler’s picture

Awesome 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.

Status: Fixed » Closed (fixed)

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

kaidjohnson’s picture

In 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.

rokat’s picture

As 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

plach’s picture