Problems/Motivations

Currently a hook_library_alter() defined in the template.php of a front-end theme is not called.

When you empty the cache so it build the theme registry, the hook_library_alter() is never called.

This is however super important when you detach completely the front-end theme from drupal and wants to redefines some jquery library versions / Or adds a new UI.

Some people on the internet claims it is possible to implements alter_hook in template.php but I could not manage to reproduces it.

The only work around right now is to externalize a module which define the alter hook, but then for my use case it replace the jquery, and jquery.ui for the whole drupal system, thus the admin theme as well.

It then break the admin theme and contributed modules when contributing on the back-end which is not the expected behavior.

Proposed resolution

Theme should be able to alter hooks on a per-theme basis.

A possible solution is having a hook registry per active theme.

I read these already

#812016: Themes cannot always participate in drupal_alter()
#591794: Allow themes to alter forms and page

Commiter note

\Drupal\Core\Asset\LibraryDiscoveryCollector doesn't support theme change on single request. If theme changes class has to be reconstruced.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Title: Front end theme does not participate in drupal_alter() hooks. » hook_library_alter() is not called for themes.

It seems hook_library_alter() is called too early for the theme system (not yet initialized) for some libraries.

Most alter hooks work and are supported in themes.

jquery_update contrib module has an option to disable the adding of the library for the admin paths. Maybe that is helpful.

You could try to check in your module for "global $theme;" as well to distinguish for admin themes.

Fabianx’s picture

Issue summary: View changes

detailing

ciss’s picture

Issue summary: View changes

Encountered this myself, and narrowed it down:

This bug only affects themes that have declared JS files in their .info.
The reason is that in _drupal_theme_initialize() the JS files get added before the theme's template.php gets included. The call stack is as follows:

  1. _drupal_theme_initialize() calls drupal_add_js()
  2. On its first call, drupal_add_js() calls drupal_add_library() to add jquery.
  3. drupal_add_library() calls drupal_get_library() to retrieve the library.
  4. drupal_get_library() retrieves all system libraries and invokes drupal_alter() on them.
  5. drupal_alter() doesn't recognize any alter implementations of the theme since the functions haven't been declared yet.
star-szr’s picture

Just discussed this with @ciss on IRC.

This still seems to be an issue on 8.x when using 'libraries' in the theme .info.yml. For whatever reason it seems like I need to disable/enable the theme to change Bartik between using 'stylesheets' and 'libraries'.

star-szr’s picture

Issue tags: +Twig
Jeff Burnz’s picture

Version: 7.x-dev » 8.0.x-dev
Component: theme system » asset library system

I think we should bump this to D8, certainly I cannot get this to fire in a theme at all, this seems rather major to me know we are being encouraged to only use libraries in themes, we really should have this firing for themes as there is basically no other way to get all the libraries, you have use hook_js/css alter to get what things done.

Wim Leers’s picture

Yep, I've always wondered how that was supposed to work: it's explicitly *documented* as working for both modules and themes, but the code doesn't match the comment.

Wim Leers’s picture

Title: hook_library_alter() is not called for themes. » hook_library_alter() is not called for themes
Priority: Normal » Major
Issue tags: +TX (Themer Experience)
dawehner’s picture

dawehner’s picture

Title: hook_library_alter() is not called for themes » [pp-1] hook_library_alter() is not called for themes
Status: Active » Postponed

.

Wim Leers’s picture

Title: [pp-1] hook_library_alter() is not called for themes » hook_library_alter() is not called for themes
Status: Postponed » Active

I think we can actually do this now.

catch’s picture

Title: hook_library_alter() is not called for themes » hook_library_info_alter() is not called for themes
lauriii’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
4.56 KB
Fabianx’s picture

Status: Needs review » Needs work

Patch generally looks good, but we will need to include the active theme in the cache collector key:

See also #2448843-28: [regression] Themes unable to implement hook_element_info_alter().

The last submitted patch, 12: 2050269-12.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.47 KB

Got a little stuck with this. Im trying to find someone who could help me move this forward.

Status: Needs review » Needs work

The last submitted patch, 15: 2050269-15.patch, failed testing.

Fabianx’s picture

Looks good to me what I see :).

lauriii’s picture

Status: Needs work » Needs review
FileSize
17.37 KB
5.73 KB

Fixed some of the tests and did some clean up

Status: Needs review » Needs work

The last submitted patch, 18: 2050269-18.patch, failed testing.

lauriii’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
17.19 KB
lauriii’s picture

FileSize
1.05 KB

Interdiff

lauriii’s picture

This comment was on wrong issue

Fabianx’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
    @@ -21,7 +22,7 @@ class LibraryDiscoveryCollector extends CacheCollector {
        *
        * @var string
        */
    -  protected $cacheKey = 'library_info';
    +  protected $cid;
    

    Lets just remove that, the parent will take care of setting this to the right value.

  2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
    @@ -56,8 +64,10 @@ class LibraryDiscoveryCollector extends CacheCollector {
    +    $this->cid = 'library_info:' . $this->themeManager->getActiveTheme()->getName();
    +    parent::__construct($this->cid, $cache, $lock, ['library_info']);
    

    Just use:

    $cid = ...;
    parent::__construct($cid, $cache, ...);

Little things regarding usage of internal properties, but then this should be RTBC. Overall looks great!

dawehner’s picture

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
@@ -56,8 +64,10 @@ class LibraryDiscoveryCollector extends CacheCollector {
+  public function __construct(CacheBackendInterface $cache, LockBackendInterface $lock, LibraryDiscoveryParser $discovery_parser, ThemeManagerInterface $theme_manager) {
+    $this->themeManager = $theme_manager;
+    $this->cid = 'library_info:' . $this->themeManager->getActiveTheme()->getName();

Constructing the cid that early feels kinda problematic. This could be really early, if something has that as part of a dependency chain, so better do that, when you actually need it.

lauriii’s picture

Status: Needs work » Needs review
FileSize
17.25 KB
1.07 KB
Fabianx’s picture

#24: That is the exact same as in ThemeRegistry and ElementInfo cache however.

So I think fixing that should be a follow-up and we need a listener to theme changes?

Edit, chat with background info:

06:18 < lauriii> Fabianx-screen: so I wanted to talk briefly about https://www.drupal.org/node/2050269
06:18 < Druplicon> https://www.drupal.org/node/2050269 => hook_library_info_alter() is not called for themes #2050269: hook_library_info_alter() is not called for themes => 20 comments, 3 
                   IRC mentions
06:18 < lauriii> Fabianx-screen: and the one last failing test
06:18 < lauriii> Fabianx-screen: which is actually the one I created for it
06:19 < lauriii> Fabianx-screen: so the problem is that the cache id is being set in the constructor
06:20 < lauriii> Fabianx-screen: and after the class has been constructed it doesn't change anymore
06:20 < lauriii> Fabianx-screen: should it be that way or should it be changed to be something else?
06:20 < Fabianx-screen> lauriii: Lets take a look at the interface.
06:20 < Fabianx-screen> lauriii: 
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21CacheCollectorInterface.php/interface/CacheCollectorInterface/8
06:22 < Fabianx-screen> lauriii: yes, I think it is correct. Why would we need to change the cid?
06:22 < Fabianx-screen> lauriii: It seems to be the same way in the 
                        https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Utility%21ThemeRegistry.php/class/ThemeRegistry/8
06:24 < lauriii> Fabianx-screen: I was just wondering if there would be such use case where library collector would be construted and the 
                 theme changes after that
06:24 < lauriii> Fabianx-screen: which would would cause libraries being loaded for the previous theme
06:24 < Fabianx-screen> lauriii: There should not be, else hook_element_info() would be wrong, too.
06:24 < Fabianx-screen> Can you write that as a committer question in the IS?
06:25 < lauriii> Fabianx-screen: yes I can write that there
06:25 < lauriii> Fabianx-screen: so someone think about it later
06:25 < Fabianx-screen> lauriii: yes
06:25 < lauriii> Fabianx++
06:25 < Fabianx-screen> lauriii: great catch, though
06:25 < Fabianx-screen> :)
06:26 < Fabianx-screen> All those services dependent on theme would need to be informed if theme changed by implementing an event or such 
                        ...
06:26 < Fabianx-screen> But I don\t think it is an issue right now.

So I don't know when we initialize this service, but I still think ThemeRegistry and ElementInfo service have the same problem as they also use the constructor.

Or otherwise asked:

- If the theme manager is available, should not the theme be negotiated then already for this to be sane?

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

- Adding blocker tag as this seems to block #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name.
- Setting to RTBC, because this is good to go.

Caveat:

A core committer will need to check #24 / #26 before commit.

I don't think it is a problem, but if it is we have a new critical anyway ...

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +D8 Accelerate Dev Days

That is the exact same as in ThemeRegistry and ElementInfo cache however.

Is it?

    $theme_name = $this->themeManager->getActiveTheme()->getName();
    if (!isset($this->elementInfo[$theme_name])) {
      $this->elementInfo[$theme_name] = $this->buildInfo($theme_name);
    }

in \Drupal\Core\Render\ElementInfoManager

I think that ThemeRegistry might have the same problem though.

Fabianx’s picture

Status: Needs review » Needs work

After discussion in IRC:

- Lets use NULL for the cid we give to the constructor
- Lets implement the cid creation logic in getCid(), LocaleLookup::getCid() already does that.
- I missed that method yesterday

Sorry for not seeing that earlier. The goal of that change is performance, so we don't init the theme system on service creation.

--

Follow-ups needed:

- Have ThemeRegistry implement getCid() as well and not call theme neg, etc. just because something injects the theme_registry service (all functions lazy load everything anyway).
- Re-factor CacheCollector to use a $this->storage[$cid] prefix for all internal storage like HookElementInfo() or implement a kind of SubscriberInterface so that a changing getCid() can be detected and handled. (needs more discussion)

lauriii’s picture

Assigned: Unassigned » lauriii

I will work on this later today

dawehner’s picture

I think that ThemeRegistry might have the same problem though.

Note: We store the theme registry entry per theme, see \Drupal\Core\Theme\Registry::getRuntime

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryCollectorTest.php
@@ -75,6 +86,17 @@ protected function setUp() {
+      ->will($this->returnValue('kitten_theme'));
+    $this->libraryDiscoveryCollector = new LibraryDiscoveryCollector($this->cache, $this->lock, $this->libraryDiscoveryParser, $this->themeManager);

@@ -90,12 +112,23 @@ public function testResolveCacheMiss() {
+      ->will($this->returnValue($this->activeTheme));
...
+      ->will($this->returnValue('kitten_theme'));

This is just a future reference: You can write this code easier by use $this->willReturnValue

lauriii’s picture

Status: Needs work » Needs review
FileSize
17.9 KB
5.18 KB

I added the getCid method there. Let's see if testbot is happy.

@dawehner: Thanks! I didn't know that. Made that change for this patch

lauriii’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

The #32 interdiff addressed #29.

lauriii’s picture

Assigned: lauriii » Unassigned

Im unassigning myself. Just nothing to do right now

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
@@ -68,6 +67,17 @@
+  protected function getCid() {
+    if (!isset($this->cid)) {
+      $this->cid = 'library_info:' . $this->themeManager->getActiveTheme()->getName();
+    }
+
+    return $this->cid;
+  }

I love that!

This is a nice idea!

--

RTBC + 1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Lets' create those followups.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed eb1ee5a and pushed to 8.0.x. Thanks!

  • alexpott committed eb1ee5a on 8.0.x
    Issue #2050269 by lauriii: hook_library_info_alter() is not called for...
jibran’s picture

Wim Leers’s picture

This made Drupal 8's frontpage almost a millisecond slower. We should investigate how to avoid that.

dawehner’s picture

Mh, #2472547: Remove deprecated hook_library_alter() should have made that faster again?

Wim Leers’s picture

Oh, indeed, yay! :)

Status: Fixed » Closed (fixed)

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