Private properties in abstract class DrupalCacheArray

Hmm, set() takes those as arguments in case an implementation wants to set the cache with something other than what's in the property, but we could probably force people to override it.

The change from protected to private looks dead right though, this will need commit to 8.x first than backport to 7.x.

I think neclimdul spotted something like this in terms of theme registry caching, I've pinged him to ask him to take a look - it's possible we could turn that into a test case.

Private has no place in Drupal. Those should have been protected from the get go.

Committed to 8.x. Moving to 7.x.

@Dries: Your commit additionally removed phpDoc, which wasn't contained in this patch and should not have been removed:
http://drupalcode.org/project/drupal.git/commitdiff/7be846e75d21c83a92ee...

Oh so that's how that got fixed in D8. Edit: See #1366804-4: "End of foo group" on a couple of API pages due to not following standards. It's a needed change, just part of a different issue. :)

I was going to ask for tests for this, but it's also an obvious issue since we don't allow private properties anyway (not sure how I ended up getting those in the original patch and past reviewers..), and since it's already committed to 8.x it's RTBC for 7.x as well then.

No that looks like an oversight - worth opening a separate issue for.

While technically an API change for D7, this is only going to open opportunities people extending the class didn't have before so there shouldn't be any harm.

I'm not sure about that "never, ever use private" rule though, or when/who/why that was added to http://drupal.org/coding-standards (my kingdom for a 'blame' feature in Diff module :)). In terms of API design, the "private" keyword is actually very useful, if that's what you want. However, it seems here we don't, so...

Committed and pushed #1 to 7.x. Thanks!

Let's add a test for this.

The patch from this issue introduces #1391364: Latest 7.x dev(1/1/2012) throws WSOD if any theme enabled while overlay module enabled., both in D7 and D8.

Reverted from 7.x and 8.x:

git revert 24c357b5c521ade1d900 # 7.x
git revert 7be846e75d21c83a92ee5e # 8.x

...by webchick, not me. :)

Closed #1391364: Latest 7.x dev(1/1/2012) throws WSOD if any theme enabled while overlay module enabled. as a duplicate. We either need to postpone this on a fix that makes ThemeRegistry not try to alter the formerly-and-again private variables in DrupalCacheArray, or roll a fix for that in with this patch. We should also come up with a unit test for that, since #1391364: Latest 7.x dev(1/1/2012) throws WSOD if any theme enabled while overlay module enabled. cannot be tested functionally.

I guess this needs to get bumped back to major as well.

Started looking at this. It looks like when the properties are protected, enabling the stark theme leads to seven's entry for the ThemeRegistry cache having only entries for toolbar module in it - so the WSOD is because as far as it's concerned, no other theme functions exist. This is due to ->set() not taking into account the fact we need a processed theme registry with NULL array keys when the cache is empty, rather than just taking whatever's set in $data. This would only ever be triggered when another request clears the theme registry cache in between the request starting and finishing, and without writing back to it which is probably why it's only ever triggered by the overlay.

We could likely simulate that in a test by initializing the theme registry class, requesting a theme function to trigger a cache write, then deleting the cache entry, then destroying the class so it calls __destruct() and tries to write to cache. Didn't get to that yet though.

While debugging this I found two further issues:

- $this->completeRegistry() was not avaialble in __destruct(). After re-initializing that variable it all works again, but that's not a proper fix for this (see patch for the hack).

While in there I noticed that l() is still calling theme_load_registry() and getting the full thing, that looks to have been lost in the final one or two re-rolls of the original issue. Ironically it is probably this omission that prevented this bug being found sooner, since any broken ThemeRegistry object is overwritten as soon as l() is called with the full version.

So @todos (at least):

- write a test that intializes the ThemeRegistry object, requests a theme function that's not cached yet, manually removes the cache item, then destroys the object, then initializes again and makes sure that all potential theme functions are available instead of just the one that was a cache miss.

- figure out wtf is going on with $this->completeRegistry() in __destruct (and probably move that array_fill_keys() logic to a helper function as well).

Got the bug while debugging lock system, having those properties as private do break the theme cache implementation.

Why does the set() method has cid and bin as parameters anyway?

That was in there to support writing to more than one cache entry without having to reimplement the locking, however in practice no implementations are using this any longer so it should go.

Here's a test that will fail properly in the same way as the one enabling a theme from inside the overlay.

CNR for the bot.

This one should pass, also refactored the array_keys() stuff into a helper method.

Missed a bit.

+++ b/core/modules/simpletest/tests/theme.testundefined
@@ -540,3 +540,56 @@ class ThemeHtmlTplPhpAttributesTestCase extends DrupalWebTestCase {
+   * Test the behaviour of the theme registry class.

Nits:

1. Should be "Tests."
2. British spelling of "behavior." :) (Edit: In getInfo() as well.)

The test-only patch was postponed on QA, so I requeued it manually. It also fails for me locally. So as I understand the test basically emulates the sequence of events for the registry when the theme is enabled from within overlay?

Edit: Another nitpick is that the assertion message "Value NULL is TRUE" is not so useful so maybe we could add custom assertion messages? Edit 2: Though I think sun would disagree on that point, and one can always check the line number, but...

Tagging novice for the task of cleaning up the three things mentioned in #29 if no one else gets to it first.

+++ b/core/modules/simpletest/tests/theme.testundefined
@@ -540,3 +540,56 @@ class ThemeHtmlTplPhpAttributesTestCase extends DrupalWebTestCase {
+    $this->assertTrue(cache()->get($cid));
...
+    $this->assertTrue($registry['theme_test_template_test']);
...
+    $this->assertTrue(cache()->get($cid));
...
+    $this->assertTrue($registry['theme_test_template_test']);
+    $this->assertTrue($registry['theme_test_template_test_2']);

To clarify my previous comment, all these assertions are missing message text, and are just using the default "Value X is Y." See: DrupalTestCase::assertTrue().

Not going to set it back over that though. :P

Say one thing and do another. Awesome.

Here it is with some messages.

Yay for more context for poor suckers scanning results on QA. :) That looks fine to me.

I reviewed the patch again and I think everything is clear, except for this:

+++ b/core/includes/common.incundefined
@@ -2336,7 +2336,7 @@ function l($text, $path, array $options = array()) {
-      $registry = theme_get_registry();
+      $registry = theme_get_registry(FALSE);

You mentioned this change was present originally in a previous issue, and was lost in its rerolls. So that fix is included here just to fix the unnecessary overhead, or is it related to the bug fix?

I also noticed that the backport will need to be more than just a simple reroll (since cache() does not exist in D7)?

Oh, and the novice part is fixed. :) Thanks @ArtistConk!

So that fix is included here just to fix the unnecessary overhead, or is it related to the bug fix?

That's just to fix the unnecessary overhead. It's related to the bug fix in the sense that it's been masking the fact this was broken in 99% of cases.

That theme_get_registry(FALSE); is a bit unclear. Should we add a comment explaining why its there so someone doesn't remove it again?

#39: I thought the same earlier, but before posting #36 I grepped core and there are one or two other other places it's used like that, with no comment. So, we could split that change off into a separate issue and add comments... or let it slide. :) Either way.

I was just making and observation and don't feel strongly either way. I'll leave it up to you since you've done a more in depth review of the patch. :)

I'm okay with it as well. RTBC I think. Depending how Dratchick feel, we can spin off a followup for to add the comments to theme_get_registry(FALSE) calls, or not.

I requeued the patch on QA so we should hopefully see green in a bit.

I think it's worth a follow-up for the theme_get_registry() calls. The other one in core doesn't have this, so to save the patch getting bigger I'd rather not add here.

Also we should consider changing the argument default in Drupal 8 (or open an issue for a more thorough refactoring of the theme registry, this only fixes part of the performance issues with it, cache misses are still particularly bad).

I've committed/pushed this to 8.x (I wrote some of the patch but it is mostly test coverage), moving to 7.x for backport.

For D7 need to decide whether to break bc in the case that someone uses DrupalCacheArray and calls ->set() from their class, I'm 99% sure that no code exists that does this except for the code touched in this issue, but it may be possible to keep the same API but still fix the bug. I'd lead towards breaking the API - it's only been in core for 2-3 months and the use case here for calling set() directly is a fairly unusual one.

Here's a first stab at a patch. I'm not too familiar with the differences between the 8.x and 7.x testing system (shame on me) so the tests need work.

I should point out that I was directed to this issue after running into problems trying to get MongoDB cache and watchdog to play nicely. In short the destructor in the ThemeRegistry class was being called but the constructor wasn't and I was getting an error. When using these MongoDB modules and ffter applying this patch I still get an error on install but now after clearing caches it is gone. Once I hit a new page the error returns.

typo in the last patch. Everything is working great now :)

Still needs tests updated.

The only obvious thing wrong in the test is:

+    cache_clear_all($cid);

this needs to be:

+    cache_clear_all($cid, 'cache')

I fixed cache_clear_all().

Basically copying from the D8 patch, I've added hook_theme() to theme_test.module and created the same placeholder theme_test.template_test.tpl.php file.

+++ b/modules/simpletest/tests/theme_test.template_test.tpl.phpundefined
@@ -0,0 +1,2 @@
+Fail: Template not overridden.
\ No newline at end of file

Needs a newline in the end

Thats it I guess...

-23 days to next Drupal core point release.

added newline

This looks exactly right as a straight backport of the Drupal 8 patch.

Note there's an API change here, it will affect modules that inherit from DrupalCacheArray and either override the set() method, or call it directly. Currently the only implementation I'm aware of in existence is the one in this patch. I also wrote every other implementation of DrupalCacheArray I'm aware of as well. This doesn't rule out someone using it somewhere in contrib or custom code somewhere, but it feels unlikely since it's pretty new, and designed for very specific use cases.

Ok, committed and pushed to 7.x. Let's try this again, gulp. :)

This needs a change notice. Catch said he is on it.

Added here http://drupal.org/node/1422264. Thanks!

Untagging.