Pretty self-descriptive. We load the theme system on every full bootstrap so should load this from cache_bootstrap instead of the generic cache bin, also there's no need to do a wildcard clear here so we should avoid that, since wildcard clears cause problems for pluggable cache backends (at least memcache).

CommentFileSizeAuthor
#3 theme_registry.patch993 bytescatch
theme_registry.patch2.15 KBcatch

Comments

moshe weitzman’s picture

moshe weitzman commented

so cache_bootstrap is for stuff that happens during FULL as well? fine with me.

locale hunk is unrelated?

catch’s picture

catch
he/him
Primary language English
commented

locale hunk is from another patch, will fix shortly.

For me anything needed on every request should go into cache_bootstrap as long as it's finite (i.e. we don't risk exceeding the 32mb standard user cache limit for apc).

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new theme_registry.patch993 bytes

Removing patch cruft, no other changes.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Code looks good. Bot will surely be happy.

dries’s picture

dries commented

The cache_clear stuff is a bit ugly. We really need to come up with a better way to support this.

moshe weitzman’s picture

moshe weitzman commented

@Dries - agreed. Perhaps add a 'Favorite of Dries' tag to #636454: Cache tag support

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#3: theme_registry.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-dev » 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.x-dev » 7.x-dev

No reason for this to be D8, it's less than a 1k patch.

tom_o_t’s picture

tom_o_t commented

#3: theme_registry.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

#5 makes it sound like Dries wanted another look at this one. Leaving for him.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work 
+++ includes/theme.inc	5 Sep 2010 15:39:51 -0000
@@ -292,7 +292,7 @@ function _theme_load_registry($theme, $b
 function _theme_save_registry($theme, $registry) {
-  cache_set("theme_registry:$theme->name", $registry);
+  cache_set("theme_registry:$theme->name", $registry, 'cache_bootstrap');

The cache is never read from the new bin.

http://api.drupal.org/api/drupal/includes--theme.inc/function/_theme_loa...

+++ includes/theme.inc	5 Sep 2010 15:39:51 -0000
@@ -301,7 +301,13 @@ function _theme_save_registry($theme, $r
 function drupal_theme_rebuild() {

Interesting. http://api.drupal.org/api/drupal/includes--module.inc/function/system_li... does not reset this cache, even though the docs for drupal_theme_rebuild() explicitly state that it should be called when modules change.

+++ includes/theme.inc	5 Sep 2010 15:39:51 -0000
@@ -301,7 +301,13 @@ function _theme_save_registry($theme, $r
+  $themes = list_themes();

The risk of potential recursion issues really worries me here. Had to step through various theme API functions to ensure that list_themes() is not able to circle back into drupal_theme_rebuild() itself.

Powered by Dreditor.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.