#315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) allowed modules to register sets of JavaScript/CSS libraries. The problem is that it's not cached. This means that if a contributed module did something really expensive with the registry, like searching for libraries on the harddrive, or did a complex alterations of them, it could potentially be a big performance hit. This issue is to cache that registry so that we don't run into these performance implications.

CommentFileSizeAuthor
#4 510334.patch5.91 KBrobloach
drupal.get_.library.cache_.patch5.58 KBrobloach

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

On one hand, you're right that there might be a module that performs heavy actions to alter libraries. Libraries API might be a good example. On the other hand, a module like that most probably has to perform many other additional actions anyway, so it needs it separate caching (and controlled cache flushing) anyway.

+  if (isset($module)) {
+    return isset($libraries[$module][$name]) ? $libraries[$module][$name] : FALSE;
   }
   else {
-    $libraries[$module][$name] = FALSE;
+    return $libraries;
   }

Usage of $name leads to a PHP notice here.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +Libraries

Tagging.

robloach’s picture

robloach
he/him
commented
Status: Needs work » Postponed

#505084: Add #attached_library FAPI property for drupal_add_library()

robloach’s picture

robloach
he/him
commented
Status: Postponed » Needs review
StatusFileSize
new 510334.patch5.91 KB

Updates to HEAD, takes sun's note about the $name into account and fixes a namespace issue.

mfer’s picture

mfer commented

/me subscribes

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

lilou commented
Status: Needs work » Needs review

HEAD is broken.

moshe weitzman’s picture

moshe weitzman commented

Really expensive libraries can use own caching. We have to end the cache explosion soon. -1.

robloach’s picture

robloach
he/him
commented

Moshe, what about modules that regex a file to find out what version of the library they have? To improve the developer experience (modules wouldn't have to cache the data themselves), as well as improve performance (calling a number of module hooks which could do expensive things like regex an opened file), we could just easily cache the library array.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Closed (won't fix)

Neither of those arguments is compelling. Adding caches rarely improves developer experience. You then have to understand what cache to clear when you are developing and debugging ... Further, the performance gain here is insignificant. It *only* could happen for an expensive operation when its module author cannot be bothered to do own caching.

This is a speculative cache. It might help under certain circumstances that are theoretical. Drupal has enough caches without implementing speculative ones.