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.
- http://stackoverflow.com/questions/7104262/i-cant-seem-to-override-a-fun...
- http://www.jide.fr/francais/use-your-custom-jquery-ui-theme-in-your-drup...
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.
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff.txt | 5.18 KB | lauriii |
#32 | 2050269-32.patch | 17.9 KB | lauriii |
#25 | interdiff.txt | 1.07 KB | lauriii |
#25 | 2050269-25.patch | 17.25 KB | lauriii |
#21 | interdiff.txt | 1.05 KB | lauriii |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedIt 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.
Comment #1.0
Fabianx CreditAttribution: Fabianx commenteddetailing
Comment #2
ciss CreditAttribution: ciss commentedEncountered 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:
Comment #3
star-szrJust 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'.
Comment #4
star-szrTagging for http://drupaltwig.org.
Comment #5
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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.
Comment #6
Wim LeersYep, 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.
Comment #7
Wim LeersComment #8
dawehner#2378737: Consider not executing hook_library_(info)_alter() *per* library, but once for all related issue which should probably be done first.
Comment #9
dawehner.
Comment #10
Wim LeersI think we can actually do this now.
Comment #11
catchChanging title after #2390707: Remove hook_library_alter() implementations.
Comment #12
lauriiiComment #13
Fabianx CreditAttribution: Fabianx for Acquia commentedPatch 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().
Comment #15
lauriiiGot a little stuck with this. Im trying to find someone who could help me move this forward.
Comment #17
Fabianx CreditAttribution: Fabianx for Acquia commentedLooks good to me what I see :).
Comment #18
lauriiiFixed some of the tests and did some clean up
Comment #20
lauriiiComment #21
lauriiiInterdiff
Comment #22
lauriiiThis comment was on wrong issue
Comment #23
Fabianx CreditAttribution: Fabianx for Acquia commentedLets just remove that, the parent will take care of setting this to the right value.
Just use:
$cid = ...;
parent::__construct($cid, $cache, ...);
Little things regarding usage of internal properties, but then this should be RTBC. Overall looks great!
Comment #24
dawehnerConstructing 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.
Comment #25
lauriiiComment #26
Fabianx CreditAttribution: Fabianx for Acquia commented#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:
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?
Comment #27
Fabianx CreditAttribution: Fabianx for Acquia commented- 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 ...
Comment #28
alexpottIs it?
in \Drupal\Core\Render\ElementInfoManager
I think that ThemeRegistry might have the same problem though.
Comment #29
Fabianx CreditAttribution: Fabianx for Acquia commentedAfter 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)
Comment #30
lauriiiI will work on this later today
Comment #31
dawehnerNote: We store the theme registry entry per theme, see
\Drupal\Core\Theme\Registry::getRuntime
This is just a future reference: You can write this code easier by use $this->willReturnValue
Comment #32
lauriiiI 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
Comment #33
lauriiiComment #34
Wim LeersThe #32 interdiff addressed #29.
Comment #35
lauriiiIm unassigning myself. Just nothing to do right now
Comment #36
Fabianx CreditAttribution: Fabianx for Acquia commentedI love that!
This is a nice idea!
--
RTBC + 1
Comment #37
alexpottLets' 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!
Comment #39
jibranRelated #2472119: Extension installers allow extensions with duplicate names to be enabled
Comment #40
Wim LeersThis made Drupal 8's frontpage almost a millisecond slower. We should investigate how to avoid that.
Comment #41
dawehnerMh, #2472547: Remove deprecated hook_library_alter() should have made that faster again?
Comment #42
Wim LeersOh, indeed, yay! :)