Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
When trying to share a database dump between coworkers we discovered that the full path to the theme is being cached in state at system.active_theme.THEMENAME. The problem is that his data is not correctly updated when rebuilding the cache and causes errors when trying to load the extension since the path to the THEMENAME.theme is wrong.
Also we have a state query on every request to load those themes, while this could also just be cached information, which is faster, especially once you use something else than the database to cache
Proposed resolution
Convert the state entry to use cache. One point of the original issue was that state never changes, but its not worth to care about this, given
that you have a netto runtime safe in case you use cache over state, which is what you should actually optimize for.
Remaining tasks
User interface changes
API changes
Data model changes
Beta phase evaluation
| Issue category | Bug because sharing databases is hard. The performance improvement side of it though is kinda a task |
|---|---|
| Issue priority | Major, because it improves every HTML page performance wise, potentially |
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 2499943-active-theme-cache-20-interdiff.txt | 1.22 KB | Berdir |
| #20 | 2499943-active-theme-cache-20.patch | 4.11 KB | Berdir |
| #19 | 2499943-active-theme-cache-19-interdiff.txt | 654 bytes | Berdir |
| #19 | 2499943-active-theme-cache-19.patch | 4.13 KB | Berdir |
| #7 | 2499943-active-theme-cache-7.patch | 4.13 KB | mikey_p |
Comments
Comment #1
mikey_p CreditAttribution: mikey_p commentedComment #2
mikey_p CreditAttribution: mikey_p commentedActually looked into this a little more the only place that theme.active_theme.THEME_NAME is even used is in ThemeInitialization::getActiveThemeByName() where it's being used as a straight cache :(
Comment #3
mikey_p CreditAttribution: mikey_p commentedPatch to switch this over to use the cache system.
Comment #4
mikey_p CreditAttribution: mikey_p commentedActual patch instead of output of `git status`
Comment #5
mikey_p CreditAttribution: mikey_p commentedComment #6
mikey_p CreditAttribution: mikey_p commentedImproved code comments.
Comment #7
mikey_p CreditAttribution: mikey_p commentedRemoved useless use of use statement.
Comment #14
larowlanIs there anyway we can test this, to ensure it doesn't sneak back in?
Comment #15
dawehnerWhat about also not storing a full path in there?
Comment #16
mikey_p CreditAttribution: mikey_p commentedIt would be awesome to not store the full path, but that's pretty hardwired into how Extension and ExtensionDiscovery work. The module handler gets around this but not storing serialized extensions, but constructing them based on the name of modules stored in the compiled container. This could work for the theme system, but there is quite a bit more calculation that goes into building the theme list, finding base themes, dependencies, engines, screenshot, regions, etc.
This is then awkwardly cached/stored in State again at system.theme.data which is shared with system_list() (which seems to only be used for themes now) and then it's also cached again in default bin with 'system_list' cache.
Ideally we'd refactor this to completely remove the usage of state here, and it may also be possible to remove system_list() as well.
I think this is a good workaround for now to remove one usage of the state system as a cache, and at least make it possible to remove full paths with a cache clear.
Comment #17
dawehnerYeah I did not wanted to discourage the actual change. I think its perfect to do that, but yeah this is something we maybe should also take care of separately.
Marked #2508435: ThemInitialization uses state, should use cache as duplicate as it would have been the same issue updated the issue summary and added a beta evalulation.
This is perfect
Comment #18
catchOn the duplicate issue I was thinking we could put this in cache.bootstrap - it's used every request and there's only one entry per theme, and invalidated very infrequently. So it's a good candidate for the chained fast backend. Patch looks great otherwise.
Comment #19
BerdirAgreed, makes sense.
Comment #20
BerdirAlso renamed the variable. We don't usually put the bin name in the variable, so renaming to just $cache.
Comment #23
BerdirApparently a random fail in Drupal\simpletest\Tests\SimpleTestBrowserTest...
Comment #24
star-szrMakes sense, back to RTBC :)
Comment #25
alexpottCrediting @catch as the creator of the dupe issue.
This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed d305a8e and pushed to 8.0.x. Thanks!