Currently, the only way to register libraries on the theme level is to use hook_libraries_info_alter(). This is problematic for two reasons (in case you want to register libraries in a theme) drupal_alter() highly depends on when it is invoked in the lifecycle of a Drupal request. If the theme layer is not initialized at the time when libraries_info() is invoked this causese the theme alter hooks to be ignored. Also, if the theme changes (e.g. because we are in the admin theme, or because we are using a theme key altering module [e.g. a context module plugin that changes the theme key based on certain conditions]) we may have invalid data (libraries) in the cache that should not be there. The same thing may happen the other way round => libraries_info() is invoked on the backend => cache is populated without invoking hook_libraries_info_alter() for the frontend theme (the frontend theme would normally register a library) => Next time we read the info from the cache on the frontend, the library that the frontend theme is trying to load is not there because it hasn't been loaded into the cache.

Therefore, I suggest that we do per-theme-key caching of libraries and directly invoke hook_libraries_info() for all themes in the theme trail of the theme that is currently active. I can provide a patch for that if you guys agree.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

Status: Active » Closed (won't fix)

After discussing this with @tstoeckler on Skype we agreed that it is much better to have a generic solution that is untangled from modules and themes in general. Followup issue forthcoming.

tstoeckler’s picture

Title: Invoke hook_libraries_info on the theme layer (theme trail) and cache libraries on a per-theme basis » Invoke hook_libraries_info for themes (and base-themes)
Priority: Normal » Major
Status: Closed (won't fix) » Active

Ahh, this issue was closed already, that's why I couldn't find it. :-)
Re-opening and re-purposing this per our short discussion at DCMunich.
Patch hopefully coming soon, unless I fall asleep in the next half hour.

tstoeckler’s picture

Status: Active » Needs review
FileSize
1.49 KB
6.24 KB

Okay, so this took longer than half an hour, because I since we now have the super cool hook_system_theme_info(), I wanted to add tests for this, but then I hit #1770902: Theme of parent site executing test leaks into all tests, which I'm pretty sure was the cause I couldn't get the tests to pass.

Anyway here is the patch and the patch with some preliminary tests only for the record. Once I remembered that we have that (yay!), I tested this with drush libraries-list and it works as expected.

sun’s picture

Status: Needs review » Needs work

RTBC with the following tweaks: (please feel free to go ahead quickly)

+++ b/libraries.module
@@ -340,13 +340,34 @@ function &libraries_info($name = NULL) {
-    // Gather information from hook_libraries_info().
+    // Gather information from implementations of hook_libraries_info() from
+    // enabled modules ...

Let's revert this :)

+++ b/libraries.module
@@ -340,13 +340,34 @@ function &libraries_info($name = NULL) {
+    // ... and the active theme (and potential base themes). This code is
+    // borrowed from drupal_alter().

"
Gather information from hook_libraries_info() in enabled themes.
@see drupal_alter()
"

+++ b/libraries.module
@@ -340,13 +340,34 @@ function &libraries_info($name = NULL) {
+ ¶

BLARGH! :-D

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.24 KB
1.07 KB

Thanks for the quick review.
Updated patch, going to commit this now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1565426-5-libraries-themes.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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