_system_theme_data() should clone the returned objects.

See also #305653: Themes disabled during update

And here is the patch.

Wow, I noticed this is only the tip of an iceberg! And my first analysis was inaccurate.

The actual problem is this one:
_system_theme_data() returns an array of objects, and it caches that array.
Every time a function modifies any of the attributes of these objects, this can have a side effect.

In this particular case, the side effect happens in system_get_files_database() called from system_theme_update(). This function is supposed to get the current status and other information from the database, and write it to each of the themes in the given array. But it will only do so for those values that are not already set. If the function is called a second time in one request, with objects that have already been processed, then it will not have an effect.

The solution is quite obvious: Don't return the original objects!!
If the system has worked previously in PHP4, then adding a "clone" statement for PHP5 should not do any damage, I think. And it might eliminate a bunch of yet unreported side effects as well (that only apply to PHP5).

And another patch..

I mark this as "needs review".
I leave it for D6 only, because D7 does not even have that function any more.

Please have a look at the latest patches in #305653: Themes disabled during update, which suggest a different solution for D7.
I think the solution proposed here might be better for D6 because it changes less, but we (or the one who is reviewing it) should at least have a look.

I'm fine with using the clone patch from #8, just thought that the David_Rothstein approach is worth mentioning.

How often do you think the filesystem scan will run if the data is not cached? In the other issue it was argued that it will only ever run 1 or 2 times. Otherwise, I would rather suggest to add a cache in drupal_system_listing(), that would only remember raw filesystem data.

PHP4 does always clone the returned objects, so it is a safe thing to do the same for PHP5.
A static cache gets totally out of hands, if other functions get the chance to mess with the values - and they will.

Ok, where else does core clone objects instead of returning the original ones in arrays?

Probably there are more places with the same issue. And I can easily imagine how this is already causing a bunch of problems, that we don't even know about yet.

We will not be able to eliminate all global state variables in Drupal. Think of global $user - this has a lot of side effects, but a lot of things depend on it.

Instead, we have to look at it case by case.
For static cache variables, we already chose to encapsulate the variable inside a function, to restrict read and write access. The idea is that only the function itself can write on the variables.
And in this case, the returned object could very well be an array instead - in which case it would be copied anyway.

-----------

EDIT:
One more way to say it:
Either we have to clone the returned objects, or return by reference. Otherwise we have inconsistent behavior between PHP4 and PHP5.

---------

I'm not aware that this is a general practice to clone objects this way in arrays.

I would say, it is not a general practice to cache an array with objects inside, unless there is some mechanism to make these objects read-only, or there is a very good reason why calling code should be allowed to modify the cache values.
In the case we are talking about, there is no such a good reason. It is just wrong thinking, nothing else.

Is this not the responsibility of the caller to clone objects if they intend to modify them?

We have full control of the called function, but zero control of the calling code (which could live in contrib, custom, whatever). Allowing the calling code to modify the cached values is careless and should be avoided, unless there is a good reason for it.

Now, what next? This is a burning issue.
The alternative to cloning would be to return arrays (maybe this would look more like a "general practice"). Doing this for D6 would break a lot of custom code.
I think the cloning is really the best option for D6.

-----

Btw, another issue which could be related:
#307756: Backport of #147000: Unify and rewrite module_rebuild_cache() and system_theme_data()
(just a guess)

@pwolanin (#28):
In #305653: Themes disabled during update it was said that we should disable the cache for D7, but clone for D6, to avoid a too heavy change in D6.

Months have passed since then, and nothing happened. I get a deja-vu with this bug for each new project i am invited to work on.
I think we should finally make a choice, and not let people sit on this bug forever.

For my own projects, it always did the trick. Reproducibly.

Some more feedback can be found over in the other issue, although I find mostly myself talking.
nilsja at #305653-164: Themes disabled during update says it did not help in his specific case (if he is talking about the same patch). So this would be one negative (although he didn't say it damages anything either. could be he just has a different problem, which needs to be solved separately).

As for Gabor's question in #22, I explained my opinion quite lengthy already, and I think I should not further annoy people by repeating it. In short: Drupal might not clone anything else yet, but it should (and implicitly did with PHP4).

@Gabor (#22):
In core modules:

../modules# grep -r drupal_clone .
./taxonomy/taxonomy.module:      $term = drupal_clone($terms[$vid][$child]);
./node/node.module:        return is_object($nodes[$param]) ? drupal_clone($nodes[$param]) : $nodes[$param];
./node/node.module:      $nodes[$node->nid] = is_object($node) ? drupal_clone($node) : $node;
./node/node.module:    if (strstr(node_view(drupal_clone($node)), $keyword) || strstr($node->title, $keyword)) {
./node/node.pages.inc:      $cloned_node = drupal_clone($node);
./node/node.pages.inc:    $output .= node_view(drupal_clone($node), 1, FALSE, 0);

In node.module, node_load(): Yes, we use drupal_clone() to not give other modules a chance to taint values in the cached node objects. Unfortunately, this does not help for nested objects. We'd need a recursive clone for that, which would be quite expensive. Or an effective readonly wrapper class (not realistic for anything < D8).

I can remember an issue with token that was caused by this incompletely cloned nodes, but can't find it atm (or don't want to look)..

I can absolutely confirm that the bug is gone as long as I have this patch applied.
Whenever I wipe the patch with a core update, the bug comes back. I then have to restore the patch manually.
(yeah I am considering to get a git workflow where I can easily merge updates and patches)

Hi.
I just had some themes disable themselves on a site where the above patch has been applied for a long time.

I do still think the logic behind the patch is reasonable, and it has helped.

But, I also think there must be something else going on.
I just wonder what this could be. I see myself in front of some hard troubleshooting :(