Themes disabled during update

subscribe

Changing this to critical as it affects sites which allow selection of different themes for their users or sites which use one of the modules that allowing different themes to be used for various pages of the site.

I'm experiencing this as well, but not only with themes - it's happening with modules also, so I'm pretty sure it's not being caused by the theme or theme settings. I've been trying to recreate the problem, but I can't tell when it's being disabled or why because it's very sporadic.

I've also noticed that when I make a change to post information visibility, by node, the changes are not reflected unless I go to the theme specific settings page, and resubmit the form, which is very odd. This may or may not be related, but I figured it was worth mentioning here.

I will keep trying to figure it out...

I am also experiencing this same problem with Drupal 6.4.

Same problem here -- it may be coincidence but it often seems to happen after updating a module.

I too am experiencing this particular problem. It's driving me crazy!

After doing some testing, it appears that the problem lies in the update.php file. If you run that, it will disable all your themes.

I've narrowed the issue down to the function system_theme_data() being called in update.php. The $themes_info array inside the _system_theme_data() function is cached and by resetting the cache, update.php respects each themes current status.

I've made a couple of patches, I'm not sure if this is the best method since these functions do work as is outside of update.php.

NOTE: These patches were created against Drupal 6.4.

Forgot to change the status of the issue.

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14508. If you need help with creating patches please look at http://drupal.org/patch/create

Let's try again.

thx

This issue also appears to be in 7.x.

I've attached an updated patch against the latest CVS for both 6 and 7, and changed the component to the update system as this seems to more accurately reflect the cause, as the function appears to work fine outside of update.php.

See also #147000: Regression: Unify and rewrite module_rebuild_cache() and system_theme_data(). Calling system_theme data will do a complete delete and insert for all themes in the system table, which creates a race condition and creates the same problem that has been described here.

Steps to reproduce for anyone trying to verify/test this:

1. Go to Site Building -> Themes
2. Make sure that one or more themes are enabled
3. visit update.php (You don't have to run any actions, just going to the page will trigger the bug)
4. Go back to Site Building -> Themes

Without the patch, enabled themes will no longer be enabled. With it, they should still be enabled.

Confirmed by visiting update.php with and without site in maintenance mode.

Actually, this should be fixed just by changing the call from system_theme_data to _system_theme_data (which is supposed to be used since we don't want to hit the database) instead of adding an unneeded parameter to system_theme_data. I confirmed this patch fixes the problem on a patched HEAD by visiting update.php. My themes were no longer disabled and it did not change the update process.

Nice. :) I didn't even try that, or think that it would work, but I suppose it makes sense. +1 from me.
(I had trouble applying the D7 patch, so I re-rolled it).

Patch in #17 should apply cleanly. It passed testing.d.org: http://testing.drupal.org/node/15099

Clarifying title

@ Dave Reid in #19
The D6 patch applied cleanly for me, and the D7 one applied, but spat out a whole bunch of other stuff I haven't seen before, and then patch crashed. It was most likely something quirky on my end. The patch does look fine though. I can't explain it.

To reproduce, enable a couple themes, go to update.php (don't need to do anything else), then revisit the themes page. Your themes are no longer enabled.

After patch, checkboxes stay checked.

Committed, thanks!

Moving back to 6.x.

subscribe

no success for me.

Unfortunately, applying the patch, change to $themes = _system_theme_data(); in update.php does NOT do any different. The Enabled check box is not checked anymore, but Default yes. So the site is still running with the theme.

ealier without realizing this, whenever configuring blocks, I get Access Denied , because no theme is enabled. err ... iam lucky I realize the issues and found this thread.

I can confirm, the D6 patch does not work.

Note: to reproduce the error, it is not enough to visit the update.php page, you need to click on the update button..

I'll look into this shortly...

Ok, wow. I hate these functions so much. Finally figured out what is going wrong. I'll try to explain as clear as I can.

• On an update page, the first relevant call is list_themes()
• list_themes() calls _system_theme_data() and then uses:
  
  <?php
    $themes = _system_theme_data();
    foreach ($themes as $theme) {
      ...
      // Status is normally retrieved from the database. Add zero values when
      // read from the installation directory to prevent notices.
      if (!isset($theme->status)) {
        $theme->status = 0;
      }
      ..
    }
?>
• When list_themes() modifies that data, those modifications are also saved to the static data variable in _system_theme_data since it is automatically returned from _system_theme_data by reference.
• Now when whenever we call system_theme_data (which calls _system_theme_data within it), all the $theme->status variables are set to 0.
• system_theme_data calls system_get_files_database on the data from _system_theme_data.
• Since the status key is set, the system_get_files_database function will not override the status key with the current status from the {system} table.
• This data (including every theme's status->0) is now saved to the {system} table within system_theme_data, disabling all themes.

Where can we go from here to solve this?
1. We implement a $reset = TRUE parameter as was proposed in #13 for _system_theme_data so that system_theme_data will always get a fresh copy and not the saved static copy that could have been modified. Any calls to system_theme_data should result in the correct data.
2. Change system_get_files_database so that if a column in the {system} table is different from a key in the module/theme data, grab the value from the {system} table record. Any calls to system_theme_data should result in the correct data.
2. Make list_themes work a little smarter and not have to set status = 0 for all the themes.

I'll work on a patch...

I hate these system_theme_data functions. A lot.

subscribe

I must admit, I didn't test the latest patch all the way through an update, I did however test the one in #13 all the way through, so I know that that solution will work, however I do not really like it, as caching is a good thing. So that rules out option number 1.

Option 3 seems impossible to achieve. At least I can't find away.

So this leaves option 2. In this light, I've put together a nice simple patch that I've tested and seems to work well.

Let me know what people think.

Moving back to 7.x

Re #29, this is more what I was thinking:

The check for !isset($files[$file->name]) is removed since it will never hit because we check isset($files[$file->name]) three lines above it.

To test this on your own:

You will see that the $unchanged_themes variable has the changes made in the $changed_themes variable. A solution to fix this issue should not let that happen.

#29 - cdale
Caching is a good thing though I do not think it is needed to the same extent as a regular Drupal page for update.php as update does not need to run all too often. The caching is what is creating the problem because somehow the $themes_info array is not getting fully set like it does on a normal Drupal page when the function _system_theme_data is called. If we can find the reason for the $themes_info array not getting fully loaded correctly, the issue will be solved.

I believe I've found the problem, although I'm not sure on the best way to fix it.

From a comment I found at php.net

On array copying a deep copy is done of elements except those elements which are references, in which case the reference is maintained. This is a very important thing to understand if you intend on mixing references and recursive arrays.

As each of the themes within the returned array is an object, a reference to the object is returned, causing any changes done, to actually affect the static $themes_info in _system_theme_data.

Does anyone have any suggestions on how we could cleanly make sure the returned objects in the array are copies? It would seem that looping might be the only way to achieve it.

Hi all,

Here's a totally different approach to the problem.

Everyone's concentrating on the fact that status gets set to 0 for these themes.

Is anyone questioning why it gets written to the database?

It's a cached copy, isn't it? Why are we doing database inserts of (arguably incorrect) cached copies which we (arguably) may not have to?

update.php calls update_check_incompatibility() which calls system_theme_data() which unconditionally always writes the theme info structure to the database, including incorrect status values.

Can we add a param $update_db = FALSE to system_theme_data() so it doesn't write to the database when it is called by update.php?

[EDIT]

Hmmm... update.php also calls drupal_flush_all_caches() which calls system_theme_data(), so that's another write to the database which needs to be neutered.

@35. The problem is still any call to either _system_theme_data or system_theme_data will return the invalid status=0. So if were trying to list themes, show current theme, do anything with themes, we're now dealing with invalid data. True, this should also avoid loading back to the database. I'm going to work on an end-all patch tonight for this issue.

@36:

Ah okay, fair enough. I didn't think of it that way.

You (and others) have spent more time on the subtleties, so I will defer.

Thank you.

Ok after a metric-crap-load of testing, I can really only see two approaches to fix this problem of the themes array being deep copied.

1. Perform a shallow copy on the themes array in _system_theme_data so that modifications to the themes array don't modify the static variable.
2. Implement a $reset parameter in _system_theme_data that will not use/trust the cached static variable in the cases that data will be written to the database.

Personally, I prefer approach #2, but let's get some more input.

Good work on sorting this all out Dave.

I'm impartial to which method is ultimately chosen, so long as it works.

Here's what I think the pro's and con's of each method are:

Method 1: Implicit Shallow Copy

Pros:

1. No change to current interface
2. users of system_theme_data do not need to consider if they need a 'clean' copy, and they don't need to worry about the case where another function/module has altered the data when perhaps it should not have.

Cons:

1. Potential for higher memory usage, and longer execution times.

Method 2: Explicit reset

Pros:

1. Caller is guaranteed to get a clean copy of the data when they request it.
2. Caller can use the static value if they don't care about the integrity of the data.

Cons:

1. To guarantee integrity, callers will have to use the reset.
2. Forces the caller to use reset when they are saving information to the database.
3. More potential for subtle bugs. (Easily fixed by adding the $reset = TRUE, but only if found.)

Given all that, I would say that I would lean more towards number 1. These are my opinions, and I'm happy to have them debated and proven wrong. :) Just looking for the best solution.

Congratulations, Dave Reid.

Tested patch #2 from comment 38 and it applies fine to Drupal 6 with a fair amount of fuzz. It fixes the problem of no theme enabled for a non-default installation profile.

I would say this is ready to go in, but I know it has to be tested and applied against Drupal 7 first.

@cdale, I think calling reset when data is involved is best– this really applies to installation and updating, so it's Drupal core's problem, correct?

I fear this will go back and forth a few more times but, already, thanks very much for work, in particular by Dave.

^{benjamin, Agaric Design Collective}

I think with #38 patch 2, you did not solved your test case in #32. The question: is it necessary to pass that test or that test is wrong? What is the expected functionality for _system_theme_data() function?

Also patch 2 is an API change. So to get accepted for D6, then it should be verified that it is the best solution.

I would like to see this bug fixed before the next release..

I've been using patch #1 and it works great. No issues what so ever.

Having looked at patch #2, if that is the method that people prefer, then I think some other changes might be required. For example, going though core, the majority of calls are to system_theme_data, not _system_theme_data, and the calls to _system_theme_data are only on maintenance pages, or pages that are not allowed to hit the db. So with patch #2, what is the purpose of leaving $themes static, as from what I can tell, $themes will be rebuilt every time they are requested under normal usage as $reset will always be true. Is this ideal?

I'm not meaning to sound overly critical, I think Dave Reid has done a great job, I'm just trying to find the best solution to a problem, and I personally feel that patch #2 is not it given the methods that system_theme_data is used throughout core.

Is there any particular reason people prefer patch #2 over patch #1?

Perhaps another approach could be to use the clone method, but only when the caller knows it will be modifying the data in a way that other calls would not desire.

I've attached a patch that I think demonstrates what I mean by that approach. The patch also reverts the change committed in #22.

Does this seem like a better idea to most?

The solution in #17 works perfectly except for when the updates are actually made, that is when the batches get processed. If we can find what is most likely calling system_theme_data and change it to _system_theme_data or if another method is to blame, fix it there.

Testing this faster in current HEAD:

Afterwards, execute update.php, update, and see no theme enabled.

As for the patches, I tend to the $reset variant. Analyzing now.

Also, in D7 use "clone", in D6 we use drupal_clone() to clone data structures.

The solution is actually trivial:

In maintenance mode, do not update theme data in the database at all.

update.php's update_check_incompatibility() already cares for disabling modules and themes that are incompatible with the current version of core, so we can safely avoid this database update.

This patch also reverts the original patch the has been committed. So after applying this patch, just visiting update.php is sufficient to see that themes are no longer disabled.

Same patch for 6.x.

Confirmed the bug and that the D7 patch fixes it. RTBC.

I think in D6 the maintenance_mode is not set at update..

this code is used in D6 for the update script check:

cross post

@Pasqualle: It is, see line 22 of update.php.

+1 from me on sun's patch. It's so simple, and works beautifully.

It still does not pass the use case in #32, but I feel that this may be outside the scope of this issue now, as this is only a problem when running in maintenance mode.

I have no clue why the D7 patch was reported to fail. Detailed results page even mentions that it was not tested in testbed at all. Patch still applies cleanly.

Why do we call system_theme_data() to begin with?

ok, after analyzing the whole process once more:

• update.php invokes system_theme_data() to gather all themes and test them for Drupal core compatibility.
• update.php also invokes _drupal_maintenance_theme(), which invokes list_themes() and _system_theme_data(). Note that those calls do not cause this bug, because _system_theme_data() does not alter the database. Only system_theme_data() alters the database.
• However, #35: When update.php finishes, we call drupal_flush_all_caches(), which invokes system_theme_data().

This effectively means that we could probably place the same or similar code into drupal_flush_all_caches(), and also ensure that we ALWAYS invoke _system_theme_data() in update.php and theme.maintenance.inc... - actually no, since we still want to flush all theme data when drupal_flush_all_caches() is invoked in maintenance mode.

So the proper place to prevent a database update with malformed values is in system_theme_data(). Additionally, I think we should avoid invocations of special maintenance mode functions where possible and leave it to the subsystem to properly handle the maintenance mode case. That helps in demystifying our install/update/maintenance mode process.

subscribing

Will the patches fix this problem on an existing DB i.e. on that is already getting the problem? Or do I need to do something manually to clean out dirty data?

@46: Don't use this patch!!

Sorry, Sun, I'm not rubbishing your patch. I'm only shouting so people don't use it.

The patch in #46 fixes update.php, but unfortunately, it breaks a fresh install of drupal.

@53: I concur with this line of questioning.

Going along the lines of reasoning of #35 and #53 ... why does drupal_flush_all_caches() want to call a function that writes to the database?

This just seems wrong (IMO). Flushing implies reading new data from the database, not writing data to the database (regardless of whether said data is correct or not).

BTW, the call to system_theme_data() in drupal_flush_all_caches() was added in includes/common.inc 1.756.2.27, between Drupal 6.2 and 6.3, for #239958: Clearing cache does not immediately reload theme's .info file

Reverting back to second option mentioned in my last summary. Works from my own testing.

To test this:

1) Go to admin/build/themes and ensure that at least one theme is enabled.
2) Execute the query in #44.
3) Visit update.php, but do not execute updates.
4) Go to admin/build/themes and confirm that the theme(s) is/are still enabled.
5) Visit update.php and execute updates.
6) Go to admin/build/themes and confirm that the theme(s) is/are still enabled.
7) Nuke your Drupal HEAD test site.
8) Install from scratch and confirm that Garland theme is enabled in admin/build/themes.

And, yes, this adds cruft to drupal_flush_all_caches(). However, I do not think that duplicating that function's contents into update.php would be a good idea.

I got a bit confused about versions of core, so just clarifying ...

My comment #57 refers to Drupal 6.6. Sun's patch #60 refers to Drupal 7.x.

Some testing:

I tried the patches from #46 and #60 together against Drupal 6.6 (I backported #60 by hand myself, as it's not meant for 6.6).

Result: update.php no longer disables theme's, but when installing a fresh installation, the theme is not enabled. Breakage.

Forget previous patches. Only apply #60. It's supposed to be applied on D7/HEAD, since D6 needs an additional tweak to get this working. However, D6 will be fixed after this patch hit D7.

sub

subscribe

subscribe

This bug has ugly side effects, at least in D6. E.g. that custom blocks created by users are not written to the blocks table in block_add_block_form_submit (as it checks for enabled themes) but only in _block_rehash. This causes the block title which the user entered in the form to be lost.
And it makes hook_block('save') in any custom module completely useless for user-created blocks because the block is not yet written.

Hoping for a soon fix in D6...

subscribe

The side effects are for all functions that lists modules. For example themes are not longer tracked by update status. The System info module gives out an empty theme list, etc.

I believe that Drupal 7 will be fixed before D6, see comment #62.

The last submitted patch failed testing.

Failed due to #74645: modify file_scan_directory to include a regex for the nomask.. Setting back to code needs review.

subscribe

subscribing

This is an easy patch to test, why not try it and report back? Might even get fixed that way...

I would like to do that but I don't have D7, only D6 and sun said that there was some additional patch needed which we get only after D7 has been fixed :-(

I can report that it works fine for my installed D6, knowing that it is supposed to break a fresh installation.

webchick, I think this should be a 7-0-UNSTABLE-4 blocker to agree on a solution and get this fixed and then backported.

This should also become a blocker for a D6.7 release.

it does not blocked D6.6 so I don't see the point why it would block the next release..
It's just a critical D6 bug as any others

Hmm, most of the post in the list you linked to are not really critical bugs as they don't influence any other stuff. And many of the post end with "Oh, found the bug, was my fault" or sth.

This bug influences a lot of other modules, like stopping custom blocks and hook_block from working correctly (just to name the side effect that affects me), and this is bit more severe than most of the other "critical" bugs in your link...

subscribe

I have noticed that theme gets disable after cron runs too! Is this related to this issue or separate (this is for drupal 6 but thought it might give you more information)

Forcing re-test of last patch.

If your system has "no theme enabled", custom blocks (and Menu block module blocks) can't save their settings because those settings are saver per-enabled-theme. :-p Pretty critical. But debating whether this is a 6.7 blocker is pointless since a security bug will always force a new 6.x release regardless of any pending critical bugs.

Too much subscribing (we really need to fix the comment-to-get-email-updates "bug") and not enough testing!

Before I applied the patch, I created a bogus book_update_6001() to test. I can confirm that running update.php will nix any enabled themes. I can also confirm that running cron.php does not disable themes, spopovits. This is with all core modules enabled and no contrib modules.

After I applied the patch, I created a bogus book_update_6002() to test. And I can confirm the patch fixes the issue.

John: You don't need to add any bogus update hooks... simply run update.php to clear caches and all themes are disabled.

subscribe

subscribe

subscribe

Committed this to HEAD. Thanks!

Moving back to 6.x.

Trivial re-roll of sun’s patch. Should be RTBC, but I’ll let someone else double check.

For D6, we also need at least the changes from the patch in #18.

Done.

The patch in #90 applied cleanly for me on two different sites running 6.6 and appears to fix the problem.

I'd say RTBC then.

subscribe, patch applied for live site which often breaks

I've open #344625: Themes disabled on cron because this patch work only for update.php which is deleted on site but after 7 hours site "forget" about custom theme again

+1 The patch in comment #90 works fine for me for both invoking update.php and a fresh install.

Committed to 6.x, thanks!

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

I'm afraid you fixed only symptoms of the bug with _system_theme_data().
See #632080: _system_theme_data() should clone the returned objects. .