Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme

This is not a critical bug, and it may not be major either.

I don't believe you should be able to subtheme with a different theme engine. This might be won't fix.

Also, there is a minor indentation issue.

This hasn't changed in Drupal 8 vs. Drupal 7, so it needs to be fixed in 8.x first, see http://drupal.org/node/767608 for why.

Looks like a very small change, but I agree with Tim Plunkett it needs a test so it doesn't regress in the future.

It should be possible to work around this bug with a very small module implementing hook_system_info_alter(), and it's not sufficiently severe to be considered major - see http://drupal.org/node/45111 for the categorization.

This should be close to what's needed for testing:

- create a new test theme that inherits from another theme in core but specifies a different theme engine. We already have test themes in core, it might even be possible to just add the different theme engine line to one of them, or copy it to a new theme and change that one line, removing any extraneous stuff.
- write an automated test that checks the theme engine is correct in system info.

The test should fail without the patch, but pass with it.

Missing an updated patch.

#2: fix_theme_inheritance_1545964.patch queued for re-testing.

D8 still ships with phptemplate, and it isn't being removed any time soon.

Was this ever backported to Drupal 7?

Based on how close the Twig initiative is to being complete, I am postponing this to be revisited before release. Please see: http://drupal.org/node/1987510

Also, bother xjm if you don't agree ;)

This shouldn't have been postponed on twig, don't think it's got anything to do with that.

We would love to switch to twig as easy as it could be in D7 and it doesn't seem to be possible with out this feature.

In D8 as far as I understand it currently all themes are now objects and I can't really find inheritance beyond the check in getActiveTheme() Line 237. So I believe that this doesn't really apply to D8 anymore because everything is handled in an independent object and if no engine is found the current engine will probably call to the engine of the base theme object?

For D7 I updated the patch to current core as a first step. Don't know if all keys should be checked seperatetly or should still be in one if().

Sorry that was the another patch... readding file.

Updating status. Maybe this doesn't need "needs backport" anymore as we got Twig in D8 and only D7 needs this feature?

Than lets change the version to D7 and reroll the patch.

This needs to be fixed or verified that the bug doesn't exist in 8.x first before anything happens in 7.x.

Added an example to the issue summary on how to "fix" base/sub-themes temporarily that use Twig for D7.

So important issue and so long and unsuccessful storyline. It reminds me another issue where you couldn't host Drupal in environments secured with ACL.
Ok, not prob, adding one more duck tape patches to my local Drupal 7 core is a matter of moments.
Thanks for the patch and for your patience @Rene Bakx.

Provided Patch is already updated with latest Drupal 7 branch. I have verified with origin/7.x, it applied successfully.

Here is a proper patch, inheritance is broken on multiple places, and theme() function itself considers a global $theme_engine and uses it for all templates.

This patch a bit more complex gets rid completely of the global $theme_engine variable, enforce drupal_find_theme_templates() function to lookup for the current theme being scanned and set its engine into the stored theme hook information, then the theme() function can fallback on the $info['engine'] information instead of trying to use a single hardcoded global variable.

The reason why this is necessary is because if you have, for example, theme A (phptemplate) and theme B (twig) which uses theme A as base them, it may happen any of these two scenarios:

1. if I load a theme A template as first theme() call, then global engine is phptemplate, and theme B templates will never be found;
2. in the opposite, if I load a theme B template in first theme() call, then the global engine is twig, and you get my point.

Per #1537050: [meta] Should we keep / improve multiple theme engine functionality? I guess this won't ever make it to Drupal 8.

And there is still an issue with the patch, core is unable to find the right theme engine to use for modules templates when you provide them with another format than .tpl.php, but that's a real problem because:

1. nothing prevents multiple engines to have the same extension, so you can't guess which one to use using the file extension in the module;
2. module template is stored, like any other hook, in the registry without the extension, so even if we would have wanted to guess, we could have not;
3. we could arbitrarily retry using all engines until it says yes, but it would be a performance killer, so this is a no-go.

So the solution is probably to attempt to lookup for a "theme engine" key in the module info file.

I'm sorry, I used the [] array syntax at once place, force of habbits...

Another patch, hope it helps:

• renamed some null to NULL;
• removed [] array syntax to make PHP < 5.4 happy;
• removed some unused variables.

RTBC - Looks great to me.

It would be awesome to get this merged. I tested it using a few different page, node and view templates and works well. I seemed to have an issue using hook_theme within my theme (or a custom module for that matter) then overriding in my sub-theme, but could have been something else unrelated to this patch.

I have a few legacy Drupal 7 sites that copy paste a TON of code from a twig styleguide into different templates. When updates are made to the components I need to sift through different template files and make sure the markup gets updated, this patch would allow some of these components to be living. I am not sure how stable TFD7 is to run on a production site, but what this patch would let me do is create a sub-theme and one by one extend some components from twig, while keeping my legacy components in phptemplate.

It took me a few hours of debug to make this patch right, and I'd love to see it merged too! Actually, we do use TDF7 on many production sites, no problems so far, but on some other projects we tend to use a custom Twig engine with a partial Symfony 3 embeded into Drupal 7, and it's working very nicely, see https://github.com/makinacorpus/drupal-sf-dic/blob/master/Resources/engi...

@pounard - is there any way to do this without patching core?

Not really, there are probable a few hacks you can do to make it work (the original tdf7 engine does a few) but it does not solve entirely the problem. Patching core is the only way to make this work as designed imho.

#44: That is correct.

Unfortunately I missed 2 things in my RTBC-review:

--

+++ b/modules/system/system.module
@@ -2606,12 +2606,12 @@ function _system_rebuild_theme_data() {
-        $themes[$key]->info['engine'] = $themes[$base_key]->info['engine'];
-        $themes[$key]->owner = $themes[$base_key]->owner;
-        $themes[$key]->prefix = $themes[$base_key]->prefix;
+         $themes[$key]->info['engine'] = $themes[$base_key]->info['engine'];
+         $themes[$key]->owner = $themes[$base_key]->owner;
+         $themes[$key]->prefix = $themes[$base_key]->prefix;

Indentation change?

--

Also this definitely needs tests.

The theme registry is very complicated and adding more untested functionality is not a good thing.

I especially would want the tests to ensure that future changes won't break this again for tdf7.

#46:

I suggest to port the nyan cat twig engine from D8, which is tests only. (core/tests/engines/nyan)

Then do the exact same setup like you did with mothership and mothertwig.

E.g. have a base theme use phptemplate and the child theme use nyan engine.

So what I would like to see is a true integration test and not just 'mocks'.

Why was this issue moved back from Drupal 8 to Drupal 7 without fixing it in Drupal 8?

Unless I'm missing something, Drupal 8 supports multiple theme engines also. Are we sure the bug doesn't exist there?

#48: I am pretty sure we need to fix in in Drupal 8 also. I had left it originally at Drupal 7 as that might be easier to work with, but yes it needs to be fixed in Drupal 8 first - using the nyan cat engine.

Seriously... Don't mean to be rude, but patch is actually working and ready (even thought you'd like more tests, there is already all the actual tests passing); please, please, don't block this for Drupal 8.

#51: I am pretty sure once we have a working patch for D7 with tests, this can be ported quickly to D8 as the code was almost not touched at all.

So I am happy to continue for 7.x for now, but we need a test.

#53: Nyan cat can be part of the patch to create the test.

It is a very simple engine and the test for it is minimal.

The D8 code is also almost D7, so a port is just copying it over I guess and it can happen in this issue.

I do agree with @Rene Bakx, extreme complexity seems out of scope, porting a D8 theme engine is not that trivial.

http://cgit.drupalcode.org/drupal/tree/core/modules/system/tests/themes/... is the code.

The .info can be used from twig for d7. And the test theme is here:

http://cgit.drupalcode.org/drupal/tree/core/modules/system/tests/themes/...

It should really not be difficult to port this.

Oh I should have looked before speaking, I guess it's simple enough, but are you sure this covers this patch test cases?

#57: I am pretty sure it should suffice to be able to define two themes one based on phptemplate and one based on nyan cat and then reproduce the bug in the IS.

Another reason I have for having tests is that while the fix looks straightforward, we want to do as minimal changes as possible in D7, so with the tests in place, we can also see whether there is an easier solution.

And chances are very high that if we have a very good D7 patch that porting it to D8 will be very simple (as most code is the same).

Ok, nevertheless, I can't work on this right now, maybe next week but I can't promise anything I'm overloaded in work.

I'm also looking forward for this, I need it.

Yea, would love to see this in

@pounard: any new status on this ?

I don't have time to write those tests at the moment, so it's not moving forward.

See next comment.

Deleted.

Looks like that last patch prevent seven_preprocess_page() function work. Looking into it.

Deleted.

I've updated the patch.

When using panels, the admin interface using Seven theme (phptemplate) must be able to load templates from a theme using another theme engine.

In order to do so, the theme engine has to be loaded or else, the following conditions in theme.inc won't never work:

        if (function_exists($theme_engine . '_render_template')) {
          $render_function = $theme_engine . '_render_template';
        }
        $extension_function = $theme_engine . '_extension';
        if (function_exists($extension_function)) {
          $extension = $extension_function();
        }
      } 

The condition:

if ($info['type'] != 'module') {

has also been removed because it's useless now.

Here's the updated patch.

We are heavily testing this patch on all our development instances. If it's ok we will soon put it in production, as we have now switched to TWIG for theme development.

Need to update a few in-development sites with your latest patch version, but aside it looks good. Some question though: you are using the list_theme() function to when initing the engines which makes me think that you are initing *all* the theme engines, is that necessary?

Hi @pounard,

I did this because, when you're using, for example a theme hook that has been defined with another theme engine (current example with Panels layouts and TFD7), Drupal has no way to know which theme is used, so, this is why I initialize all the theme engines.
I took care of not loading them twice as you can see.

Maybe it's not the best option, if you have a better idea let me know, I'm really willing to push this patch forward.

On the other hand, I also think that the conditions:

if (isset($current->owner)) {

and

if (isset($current->engine)) {

are not needed anymore.

What do you think ?

Thanks.

Dear @FabianX,

Here's a patch for Drupal 8.2.x.
Let me know if I'm on the right track.

Thanks!

Hi,

Here's a patch for Drupal 7, including tests.

There's only one test failing, and I'm willing to get help on this one.
I'm not able to reproduce it when I do the step manually on my local instance.

Thanks.

Failed to include theme.inc seems weird, are you sure the patch apply on current 7.x branch?

Here's an updated patch for Drupal 7.

While doing the tests, I noticed that it was impossible to have a theme engine in a module.
So I wrote a hook for this. It's mostly based on hook_system_theme_info() but for theme engines.

There's still only one tests failing but I'm unable to find why yet.

I guess it's a matter of time.

This looks RTBC to me.

It's not green yet :)

Yes, but I do approve the code :) Even though some minor errors could remain, tests will tell.

Little typo in template override test

Could we move the adding of theme engines via modules to its own issue (including adding nyan cat theme engine).

I do approve of that change (Nice work!), but it should live in its own issue.

Also the D8 coverage (nice work in proving that D8 is not affected should not go to waste and live in a new child issue, normal task).

Plan of attack:

Child issues for:

- a) D8 test coverage (now that you have written it it would be a shame to get that to waste).
- b) Opening an issue for adding nyan_cat engine (and in that also adding the modules to define theme engines) and referencing that as backport from D8 (reference the D8 issue as parent).

Thanks!

Woops, the test was failing and it was totally intended, so forget my addition

@Fabianx
It don't think there is any benefit in splitting, it will only make the whole process longer, and we'd in the end, end up with the exact same change set; but by splitting it we make it harder to understand the purpose of the whole, and external viewers or followers would probably not get why this is important.

Moreover, it will also make it harder for people to apply the patch set waiting for it to be in core if it's splitted into many issues.

And, I would argue that now that it works, I don't see why we should roll it back and start all over again, it's just a complete loss of time, time that it is already hard to find hence to give to Drupal core.

I think we should keep this an atomic change and commit it as a whole, since everything in there is heavily coupled. Just write two lines into the commit message instead of one and everyone's gonna be happy with it :)

Just to be clear, 4 persons already gave 8 hour each of their time to make it work. It now has full unit tests coverage, it backports whatever you asked us to backport, it has real life production site use case, and I seriously don't have time to play issue hide and seek right now. This is a good quality patch, let's finish this for once.

Hi,

Updated patch with green tests.

Thanks @sebcorbin & @pounard for finding the issue.

Looks like there were some files missing in the previous patch.

@Fabianx: although I understand forward-porting to D8 merits its own issue, I don't see why we should open a child issue just for the sake of adding test data, i.e. only the file modules/simpletest/tests/themes/engines/nyan_cat/nyan_cat.engine, that would be a patch without tests and without logic.

After some time of pair debugging with @SebCorbin and phone conf with the 2 of us and @Pol, regarding that this latest patch includes all the things we agreed on us 3 altogether and that the patch passes tests, and fixes our bugs (at the exceptions of the Panels problem @Pol experienced, but that we agreed to fix in another issue, as it is unrelated), I'd say it's now RTBC.

Really @Pol many thanks for the time you took for this :)

@Fabianx it's your call now, I think that @Pol will open the aforementioned issues you'd like to see, nevertheless I do think this patch is ready.

Adding nyan cat and the new hook is a task, this is a bug fix.

Unfortunately those need to be split up. If this patch is truly ready however, it means that only the bug fix and the test for the bug fix need to be removed from the last patch.

The patch can be left here for all to download until the child issue is committed.

Hi,

As request by @FabianX, I've splitted the patch in different issues.

Here's the processing order to follow to fix this issue once for all:

1. Add the tests in Drupal 8: #2825382: Add new tests for testing themes and subthemes engine inheritance
2. Add a module defined theme engine for testing purposes: #2825396: Enable modules to define theme engines
3. Add this patch.
4. Profit. \o/

In this issue, tests will stay red until the previous issues are committed.

Adding nyan cat and the new hook is a task, this is a bug fix.

It's actually the test of the bug fix, not a task.

+++ b/modules/simpletest/tests/theme_test.module
@@ -27,10 +31,20 @@ function theme_test_system_theme_info() {
+  $themes['test_theme_nyan_cat_engine'] = drupal_get_path('module', 'theme_test') . '/themes/test_theme_nyan_cat_engine/test_theme_nyan_cat_engine.info';
...
+ * Implements hook_system_theme_info().
+ */
+function theme_test_system_theme_engine_info() {
+  $theme_engines['nyan_cat'] = drupal_get_path('module', 'theme_test') . '/themes/engines/nyan_cat/nyan_cat.engine';
+  return $theme_engines;
+}

+++ b/modules/simpletest/tests/theme_test.template_test_engine.nyan-cat.html
@@ -0,0 +1 @@
+Success: Template overridden with Nyan Cat theme. 9kittens

These parts also belong in the child issue.

Only the subtheme and bugfix for subtheme with different engines belongs in this issue.

Would have been easier to commit both in the same patch, we wouldn't have this kind of problem :)

Actually, the first one is related to this patch, since the testing theme exists only for our tests, as of now, but considering the second one, yes, it belongs to the other patch.

And here it is, rebased on https://www.drupal.org/node/2825396#comment-11769346

So, not switching to needs review, I do wait for the other to be commited (else this one would fail).

I am splitting the patches, but in order to gain time, including here the whole patch that includes #2825396: Enable modules to define theme engines for testing purposes.

Hi all,

A small recap on this issue since the last days.
Pounard, Fabian and me worked on fixing the issue.

We had to split the issue in multiple issues. While splitting we discovered underlying stuff that needed to be fixed.

Here's the issue list:

• #1545964: Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme (April 24, 2012)
• #2825396: Enable modules to define theme engines (blocker) (November 7, 2016)
• #2826661: Let Drupal templates use single filename extension (blocker) (November 11, 2016)
• #2825382: Add new tests for testing themes and subthemes engine inheritance Only for Drupal 8 and this is not a blocker.(November 7, 2016)

Once the children are done, we will redo the patch in this issue with only the relevant part.

@Rene Bakx yes, me too! You shot first at this bug, I hope you will get the credits you deserve as well :)

I don't think that https://www.drupal.org/node/2826661 is a blocker for this issue as this (double extension) is a limitation that has been present since a long time already, so this can proceed now.

Of course it failed, it needs to be rebased upon #2825396: Enable modules to define theme engines and #2826661: Let Drupal templates use single filename extension if possible.

Now that #2825396: Enable modules to define theme engines has been committed, here's an updated patch.

Weird, few of the fails seem unrelated, they could be related but I ran them a second time to be sure.

+++ b/modules/simpletest/tests/themes/test_subtheme_nyan_cat/test_subtheme_nyan_cat.info
@@ -0,0 +1,5 @@
+name = 'Test subtheme with base theme and another theme engine'
+description = 'Test subtheme with base theme and another theme engine.'
...
+base theme = test_theme_nyan_cat

As far as I understand this uses the wrong base theme as it contradicts the description.

I imagine a test-only patch would be working and not failing.

Could we attach a test-only patch to ensure the tests test the right thing?

Hum I reviewed the patch, and refactored the tests multiple times, I'm quite sure it does test right thing, but it's highly probable that english wording is wrong, indeed, we are both Pol and me not native english speakers.

Agree for the test-only patch, it's a good idea. I could be wrong too, I was probably too focused on the subject while debugging to have the best overview.

Hi,

@Pounard, your patch in #101 is missing stuff that prevent the tests to be green, this is why I refactored it.
Regarding the wording, I agree.
Now, I don't understand why we have to make a tests-only patch. It will obviously fail right ?

Anyway, here's the patch.

Now, I don't understand why we have to make a tests-only patch. It will obviously fail right ?

Actually, I do agree, because this patch has been worked for quite some time now, but ideally it's more a methodology than the saint graal, a test that fails proves that there's something to fix. But in the end, nothing proves that the test itself it right :)

I wouldn't be fair saying "yeah a test only patch is the best thing to do!" but in real life, it will reinsure Fabianx and other reviewers, so why not.

Okay, let me try to understand that:

In the IS the use case given is 'mothership' with 'mothertwig' as sub theme.

mothership uses php template and mother twig wants to use twig, but that fails due to the inheritance of the theme engine overwriting it.

If we test with a base theme of test_theme_nyan_cat would that not mean that we test with 2 themes that use both nyan_cat as engine?

That might still be useful in itself to test that, but unless I misunderstand something I don't think the test case is testing the bug described in the IS.

Thanks for the tests-only patch.

If we test with a base theme of test_theme_nyan_cat would that not mean that we test with 2 themes that use both nyan_cat as engine?

It actually tests that with no engine set in the info file, it gets the parent theme engine.

• ThemeEngineNyanCatTestCase tests that the .nyan-cat.html file overrides the .tpl.php one.
• ThemeEngineNyanCatSubThemeTestCase tests that the subtheme, with no engine set, does inherit from the parent them if different from phptemplate

Two tests are missing:

• Changing with an additional theme engine will ensure that with the following three engines: a -> b -> c resolution will work in reverse order for templates (if c theme overrides b, then c template is displayed, if b overrides a then b template is displayed, if no override a template is displayed). This test could be written with a single page that displays 3 templates actually.
• And another one is missing too: given a module with a phptemplate template, the nyancat engine is able to override it, and given a module with a nyancat template, a phptemplate engine is able to override it. And given that, in both case, no override exists, the original template is displayed.

The two tests I'm adding here are actually the one we wish to test: indeed, the real original bug is that Drupal can NOT handle two engines in the same runtime, and what it fixes is that Drupal will now be able to use multiple engines on the same page.

And once that is done, this should be it. I will do this maybe this weekend.

The reason why I was not worrying about the tests is because I have multiple projects in production as of today that actually do all this things altogether using the first patch before we started including the tests in it.

Hope this will be the last one, I added a few meaningful tests.

Thanks for the additonal tests, pounard!

You're welcome, I had a couple of minutes to kill during an SQL import. This sounds very much RTBC in my opinion, just not switching the state myself since I wrote the patch.

So, how is it going ?

Assigning back to myself for review.

Is this still stalling ?

It is definitely a target for 7.60 - I just did not have the time, yet to review this further, but holidays are coming.

Hi there,

Any news on this ?

Thanks.

Yes, I would like very much to have this in core so I don't need to patch all my sites anymore :)

Any hope to get this soon?

Along all the way to #129, many patches were posted, but one change, and the most important one, has sadly been dropped from the original patch: a fix that ensures that a parent theme with another engine than it's parent will keep its own instead of being forced to use the parent:

diff --git a/www/modules/system/system.module b/www/modules/system/system.module
index 53844d8..ed3f905 100644
--- a/www/modules/system/system.module
+++ b/www/modules/system/system.module
@@ -2616,16 +2616,14 @@ function _system_rebuild_theme_data() {
       $themes[$base_theme]->sub_themes[$key] = $themes[$key]->info['name'];
     }
     // Copy the 'owner' and 'engine' over if the top level theme uses a theme
-    // engine.
-    if (isset($themes[$base_key]->owner)) {
-      if (isset($themes[$base_key]->info['engine'])) {
-        $themes[$key]->info['engine'] = $themes[$base_key]->info['engine'];
-        $themes[$key]->owner = $themes[$base_key]->owner;
-        $themes[$key]->prefix = $themes[$base_key]->prefix;
-      }
-      else {
-        $themes[$key]->prefix = $key;
-      }
+    // engine and the current theme has no engine itself.
+    if (isset($themes[$base_key]->owner) && !isset($themes[$key]->owner)) {
+      $themes[$key]->info['engine'] = $themes[$base_key]->info['engine'];
+      $themes[$key]->owner = $themes[$base_key]->owner;
+      $themes[$key]->prefix = $themes[$base_key]->prefix;
+    }
+    else {
+      $themes[$key]->prefix = $key;
     }
   }

I think there's a missing test case somewhere, and now that I did upgraded to the latest patch on a new site, I encountered the bug.

Here is the updated patch.

Voila! Seriously ouate de phoque the theme system is terrible, terrible.

I would like to fix it for good, but it's not possible, the 'engine' and 'owner' key are sometime in use from the $theme structure, and sometime from within the $theme->info array, in various/multiple places in system.module, module.inc, theme.inc and some other places around.

No, I will not fix this, it would make a huge patch. But I have fixed the inheritance now, I guess, and tests are all passing locally.

My problem compared to the patch in #129 is that because 'phptemplate' was set a default engine on all themes, the condition for inheritance were not always met, and a few other edge cases happened on my local box.

All good for me.

Hum I experience some PHP warnings when I run updatedb in drush, I guess there are a few things to fix in my patch.

And here it is an updated version, with the following changes:

• _system_rebuild_theme_data(): do not store 'owner' key in serialized system info if missing, it will be stored in system table directly
• _system_rebuild_theme_data(): force engine propagation into serialized info, it can only be stored there
• system_list(): do not attempt to fix the theme structure when loading it in, since _system_rebuild_theme_data() already did all the checks
• _drupal_theme_initialize(): do not attempt to load theme owner if it's undefined in since it supposed to have been correctly initialized in _system_rebuild_theme_data()
• various other places within theme.inc: use the correct 'ower' key from the theme structure, and the 'engine' key from the info array, normalze the usage of both as much as possible.

I managed to actually remove code from a few functions, that was definitely unnecessary, since it was attempting to fix data that was already fixed by _system_rebuild_theme_data(): good side of things is that it actually reduces normal site runtime operations (but don't yay because it's very very insignificant on performances, like a drop of water in the sea). But hey, it makes it more readable in the end.

This should be the last patch, it fixes all my known bugs, all tests are still passing, and I documented code all over the patch to avoid people like me to search for hours for things that in the end, seem trivial, but definitely are not when it's not documented.

Hope the test passes on the testing environments, finger crossed :)

Wow nice!

Can't wait to test!

Thanks!

Weird thing, tests are passing on my box.

EDIT1: Actually they seem to be unrelated to theme system itself, but happen in various other tests, I would guess that something gets wrongly initialized in those conditions.

EDIT2: Only during upgrade tests, I'm debugging it.

EDIT3: Found it, during upgrade path from 6.x to 7.x, 6.x themes might not carry an 'engine' info key. I even think this is a problem of the tests themselves, although I'm not really sure about this.

EDIT4: Quite sure this is a problem with the upgrade path tests since Drupal 6 default themes are well formed and all carry the 'engine' key. But that's not a bug for this issue to fix. Writing the workaround as I speak.

This should do it (I hope). Tests haven't finished running on my box, but I think that some previously failing have passed.

1. +++ b/modules/simpletest/tests/theme.test
   @@ -677,3 +677,122 @@ class ModuleProvidedThemeEngineTestCase extends DrupalWebTestCase {
   +    // Get the base theme name from the theme 'test_subtheme_nyan_cat'
   

   Comments should (noramlly) begin with a capital letter and end with a full stop / period .

2. +++ b/modules/simpletest/tests/theme.test
   @@ -677,3 +677,122 @@ class ModuleProvidedThemeEngineTestCase extends DrupalWebTestCase {
   +    // Get the base theme
   

   Comments should (noramlly) begin with a capital letter and end with a full stop / period .

Seriously?

Thank you :)

If that matters, I just reapplied properly patch from #156 in a development site, working great so far. Still RTBC but I won't RTBC myself since I also wrote the most changing patch :)

One more site working with it.

If these bugs affect Drupal 8, they still need to be fixed there first. That's been mentioned several times in this issue. It appears to me that most of these bugs do affect Drupal 8 (with the possible exception of the one that's tested in #2825382: Add new tests for testing themes and subthemes engine inheritance), so based on that this can't be ready to commit to Drupal 7 yet.

To determine that they probably affect Drupal 8, I looked through changes in the patch here and then found the corresponding code in Drupal 8:

• The code which calls the theme engine "init" hook in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Theme%21T... always uses the active theme engine, not any of the base theme engines.
• Similar scenario as above for the theme engine "render_template" and "extension" hooks in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Theme%21T...
• The code in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Theme%21R... that processes the base theme hooks does it using the active theme engine, not the base theme engine.

I stopped looking after that, but there may be more.

***

In any case, the Drupal 7 patch itself looks pretty nice and I think there are some great improvements in there. But a few issues I noticed while reading through it:

1. -  // Initialize the theme.
   -  if (isset($theme->engine)) {
   -    // Include the engine.
   -    include_once DRUPAL_ROOT . '/' . $theme->owner;
   -
   -    $theme_engine = $theme->engine;
   -    if (function_exists($theme_engine . '_init')) {
   -      foreach ($base_theme as $base) {
   -        call_user_func($theme_engine . '_init', $base);
   -      }
   -      call_user_func($theme_engine . '_init', $theme);
   -    }
   -  }
   -  else {
   -    // include non-engine theme files
   -    foreach ($base_theme as $base) {
   -      // Include the theme file or the engine.
   -      if (!empty($base->owner)) {
   -        include_once DRUPAL_ROOT . '/' . $base->owner;
   -      }
   +  // Initialize all themes and engines.
   +  // Make sure of loading the current theme first so preprocess functions are
   +  // loaded properly.
   +  foreach (array($theme) + list_themes() as $current) {
   +    // Do not attempt to load en empty owner, this could happen with malformed
   +    // theme info, or missing parent theme engine; during runtime initialization
   +    // we should not let Drupal emit warnings on a live site.
   +    if ($current->owner) {
   +      include_once DRUPAL_ROOT . '/' . $current->owner;
        }
   -    // and our theme gets one too.
   -    if (!empty($theme->owner)) {
   -      include_once DRUPAL_ROOT . '/' . $theme->owner;
   +
   +    if (function_exists($current->engine . '_init')) {
   +      call_user_func($current->engine . '_init', $current);
   

   Several things about the above:

   • Why is this running for all themes returned by list_themes() (including even themes that aren't active on this page request and that are disabled)? That doesn't seem right compared to the behavior of the previous code (which only initializes the active theme and its base themes), and looks like it could have bad side effects since the "init" code is what loads each theme's template.php files. The best I can figure out is that this might be intended as a workaround to try to ensure all theme engines are initialized before they are used?... but if we even need to fix that in this issue, I don't think this is an effective way to accomplish it (better to have some kind of registry of which engines have been initialized and then do just-in-time initialization of any new/unexpected ones).
   • Also, the array($theme) + list_themes() construct does not look right and is going to result in this running twice for that particular theme.
   • And why is this changing the order in which the init hooks run (from "active theme last" before the patch, to "active theme first" after the patch)? The code comment about "so preprocess functions are loaded properly" doesn't mean anything to me.

2. -  global $theme_info, $base_theme_info, $theme_engine, $theme_path;
   +  global $theme_info, $base_theme_info, $theme_path;
   

   I don't think we can remove this global variable - it's a documented part of the public API. Why not just leave it in place and continue to set it to the correct value, even if core itself no longer uses it anymore?

3. -function _theme_load_registry($theme, $base_theme = NULL, $theme_engine = NULL, $complete = TRUE) {
   +function _theme_load_registry($theme, $base_theme = NULL, $complete = TRUE) {
   

   Although this is not a function that is expected to be called publicly, I still don't see any reason to change the API. Why not just mark $theme_engine as a deprecated parameter - or maybe even have it continue to behave the way it used to but have the calling functions in core always pass nothing in for it?

4. -  foreach (list_themes() as $theme_info) {
   +  $current_theme = NULL;
   +  $current_engine = NULL;
   +  foreach (list_themes() as $key => $theme_info) {
        if (!empty($theme_info->base_theme)) {
          $theme_paths[$theme_info->base_theme][$theme_info->name] = dirname($theme_info->filename);
        }
   +    $theme_path = dirname($theme_info->filename);
   +    if ($path === $theme_path) {
   +      $current_theme = $current_theme;
   +      $current_engine = isset($theme_info->engine) ? $theme_info->engine : NULL;
   +    }
   +    else if (!$current_theme && 0 === strpos($theme_path, $current_theme)) {
   +      $current_theme = $current_theme;
   +      $current_engine = isset($theme_info->engine) ? $theme_info->engine : NULL;
   +    }
   

   I am not sure what is going on here (it could certainly use a code comment), and in particular $current_theme always seems to be NULL no matter what (unless I'm reading this wrong it's only ever reassigned via $current_theme = $current_theme???!!!!)

5. @@ -2616,16 +2629,17 @@ function _system_rebuild_theme_data() {
          $themes[$base_theme]->sub_themes[$key] = $themes[$key]->info['name'];
        }
        // Copy the 'owner' and 'engine' over if the top level theme uses a theme
   -    // engine.
   -    if (isset($themes[$base_key]->owner)) {
   -      if (isset($themes[$base_key]->info['engine'])) {
   -        $themes[$key]->info['engine'] = $themes[$base_key]->info['engine'];
   -        $themes[$key]->owner = $themes[$base_key]->owner;
   -        $themes[$key]->prefix = $themes[$base_key]->prefix;
   -      }
   -      else {
   -        $themes[$key]->prefix = $key;
   -      }
   +    // engine and the current theme has no engine itself.
   +    // Note that 'engine' must be stored into the info array because it has no
   +    // associated column in the {system} table whereas the 'owner' column
   +    // exists.
   +    if (isset($themes[$base_key]->engine) && !isset($themes[$key]->engine)) {
   +      $themes[$key]->engine = $themes[$key]->info['engine'] = $themes[$base_key]->engine;
   +      $themes[$key]->owner = $themes[$base_key]->owner;
   +      $themes[$key]->prefix = $themes[$base_key]->prefix;
   +    }
   +    else {
   +      $themes[$key]->prefix = $key;
        }
   

   Since (as the code comment says) $themes[$key]->engine never winds up in the database, what is the purpose of changing this function to set that at all? Currently, it looks to me like list_themes() is responsible for setting this property, which it does always (regardless of whether it gets its data from _system_rebuild_theme_data() or from the database) so it's not obvious to me what the benefit is of also setting it in _system_rebuild_theme_data().

6. +class ThemeEngineNyanCatTestCase extends DrupalWebTestCase {
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Theme engine test',
   

   (relatively minor) There's already another test case right above this with the same "Theme engine test" name. One or both should be renamed to something more specific.

7. +    array('#theme' => 'theme_test_engine_inheritance_phpt_overriden'),
   +    array('#theme' => 'theme_test_engine_inheritance_nyan_overriden'),
   

   (minor) Here and many other places... The correct spelling is "overridden".

@David_Rothstein As I very much would like to clean this up, I have no time to do it, latest iteration cost me a full day; not counting the previous ones, for such a small bug. I'll try to give accurate answers, please excuse me if I am wrong about some points, because it's been months since I didn't touch this.

Why is this running for all themes returned by list_themes() (including even themes that aren't active on this page request and that are disabled)? That doesn't seem right compared to the behavior of the previous code (which only initializes the active theme and its base themes), and looks like it could have bad side effects since the "init" code is what loads each theme's template.php files. The best I can figure out is that this might be intended as a workaround to try to ensure all theme engines are initialized before they are used?... but if we even need to fix that in this issue, I don't think this is an effective way to accomplish it (better to have some kind of registry of which engines have been initialized and then do just-in-time initialization of any new/unexpected ones).

It does not initialize all themes, but their engines. It's legit to initialize all engines since we are not going to know which one we'll use at this point, and we need them to be early initialized. This old Drupal spaguetti code is terrible, sadly, and that's the best I could come up with. Best we can do here, I supposed, is to avoid double engine initialization.

Also, the array($theme) + list_themes() construct does not look right and is going to result in this running twice for that particular theme.

Once again, engines, not themes, and yes I guess the array($theme) + is not necessary anymore.

And why is this changing the order in which the init hooks run (from "active theme last" before the patch, to "active theme first" after the patch)? The code comment about "so preprocess functions are loaded properly" doesn't mean anything to me.

Hum, wrong comment, too many iterations on this patch. The comment should be dropped. The order here means nothing, because it's about engines and not themes.

I don't think we can remove this global variable - it's a documented part of the public API. Why not just leave it in place and continue to set it to the correct value, even if core itself no longer uses it anymore?

We could leave the variable, but it won't mean anything anymore in the case you have a theme parenting tree with more than one engine, but OK, I don't care about keeping the global here.

Although this is not a function that is expected to be called publicly, I still don't see any reason to change the API. Why not just mark $theme_engine as a deprecated parameter - or maybe even have it continue to behave the way it used to but have the calling functions in core always pass nothing in for it?

It'd just keep an inconsistent method signature, the theme code is so ugly and so old, I'd like to keep the unused variable removal. Let me explain: I know 7.x core pretty deeply, and I have a few years of patching, and core contributing behind me, and yet I had to literally step debug a complete day to fully understand what this central method was really doing. Let's not keep the cruft it doesn't help anybody.

I am not sure what is going on here (it could certainly use a code comment), and in particular $current_theme always seems to be NULL no matter what (unless I'm reading this wrong it's only ever reassigned via $current_theme = $current_theme???!!!!)

Fun thing, I am not sure myself :) There's a few typos in there, that would explain why I had some troubles with unit tests. I might have to rework this part.

Since (as the code comment says) $themes[$key]->engine never winds up in the database, what is the purpose of changing this function to set that at all? Currently, it looks to me like list_themes() is responsible for setting this property, which it does always (regardless of whether it gets its data from _system_rebuild_theme_data() or from the database) so it's not obvious to me what the benefit is of also setting it in _system_rebuild_theme_data().

list_theme() does stuff at runtime whereas _system_rebuild_theme_data() will cache the info array in the database, hence the code being moved in here: now that we are initializing theme engines, we gain a bit of speed in there. And another thing to consider is that in actual stable core, it's unclear who and when 'engine' and 'owner' are initialized: the whole point of this code, even if too verbose, is to make it explicit and at one and only one single place.

Hi,

Here is some modifications to avoid 'warning' if you apply the patch before you install Drupal

@simon.mellerin your patch seems to contain (and create) the file `old.patch`

Wondering what patch file should I be using?

Bumping target based on #161 mentioning this needs to be fixed in D8 first.