When you are using a different theme engine than php-template in your sub theme that extends a base theme that uses php-template, the system module copies the engine and owner of the base theme over in the child theme. This results in the child theme not using the engine defined in the .info file.

For example, I created a subtheme called mothertwig which extends mothership

name = Mothership
description = The Motheship is a basetheme for cleaning up crufty markup
screenshot = screenshot.png
engine = phptemplate
core = 7.x
php = 5.2
name = mothertwig
description = TWIGalized version of the mother
screenshot = screenshot.png
engine = twig
core = 7.x
php = 5.2
base theme = mothership

The result was that if you did a var_dump of list_themes()—both themes where assigned to php-template.

Possible work around for sub-themes using Twig for D7

Add the following to the template.php (7.x) file:

/**
 * Implements hook_system_info_alter().
 */
function mothertwig_system_info_alter(&$info, $file, $type) {
  if ($type == 'theme' && module_exists('tfd7') && ($info['name'] === 'mothership' || $info['base theme'] === 'mothership')) {
    $info['engine'] = 'twig';
  }
}
CommentFileSizeAuthor
#164 interdiff.txt675 bytessimon.mellerin
#164 1545964-164-theme_engine_inheritance.patch26.21 KBsimon.mellerin
#156 interdiff.txt627 bytesMunavijayalakshmi
#156 1545964-155-theme_engine_inheritance.patch24.82 KBMunavijayalakshmi
#153 1545964-153-theme_engine_inheritance.patch25.22 KBpounard
#149 1545964-149-theme_engine_inheritance.patch25.16 KBpounard
#145 1545964-144-theme_engine_inheritance.patch24.47 KBpounard
#143 1545964-143-theme_engine_inheritance.patch22.56 KBpounard
#129 1545964-129-theme_engine_inheritance.patch21.32 KBpounard
#120 1545964-tests-only.patch5.74 KBPol
#113 1545964.patch15.12 KBPol
#102 1545964-102-theme_engine_inheritance-FULL.patch19.02 KBpounard
#101 1545964-101-theme_engine_inheritance.patch15.35 KBpounard
#96 1545964-96.patch16.14 KBPol
#92 1545964-92-drupal7.patch21.48 KBPol
#90 1545964-90-drupal7.patch18.08 KBPol
#85 1545964-85-drupal7.patch22.05 KBSebCorbin
#85 interdiff-do-not-test.patch688 bytesSebCorbin
#79 1545964-79-drupal7.patch22.06 KBPol
#76 1545964-76-drupal7.patch19.61 KBPol
#75 issue-1545964-75-drupal8-2-x.patch2.94 KBPol
#71 issue-1545964-70.patch10.75 KBPol
#71 interdiff-70.diff1.92 KBPol
#70 issue-1545964-69.patch10.73 KBPol
#70 interdiff-69.diff1.89 KBPol
#67 issue-1545964-65.patch10.64 KBPol
#67 interdiff-65.diff1.8 KBPol
#64 interdiff-64.diff951 bytesPol
#64 issue-1545964-64.patch10.15 KBPol
#38 1545964-38-engine_inheritance_fixed.patch9.95 KBpounard
#32 1545964-32-engine_inheritance_fixed.patch10.02 KBpounard
#21 D7-theme_engine_inheritance-1545964-20-D7.patch892 bytesKars-T
PASSED: [[SimpleTest]]: [MySQL] 41,508 pass(es). View
#20 paragraphs-entitycache-2293549-1-D7.patch383 bytesKars-T
#2 fix_theme_inheritance_1545964.patch893 bytesRene Bakx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_theme_inheritance_1545964_0.patch. Unable to apply patch. See the log in the details link for more information. View
#1 fix_theme_inheritance_1545964.patch893 bytesRene Bakx
PASSED: [[SimpleTest]]: [MySQL] 38,710 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Rene Bakx’s picture

FileSize
893 bytes
PASSED: [[SimpleTest]]: [MySQL] 38,710 pass(es). View

The attached patch fixes this issue by adding an extra condition that checks if the base engine matches the child engine.

Rene Bakx’s picture

Status: Active » Needs review
FileSize
893 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_theme_inheritance_1545964_0.patch. Unable to apply patch. See the log in the details link for more information. View

Let me try that again. (must learn to read manuals before being excited about delivering a patch first)

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Major
Status: Needs review » Needs work
Issue tags: +Needs tests, +needs backport to D7

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.

Rene Bakx’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

i do not agree so i'am settings this back to D7 instead of D8. It is one extra if that can save a lot of time to themers. Instead of having to duplicate a truckload of code and settings between two themes just becaiuse they have a different output engine.
Having the option to extend a php-template base theme in a different engine is rather essential for the acceptence of non php-template engines amongst themers. That is something I learned last week at FrontendUnited while explaining and discussing the advantages of using twig as a template system.

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Normal
Status: Needs review » Needs work

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.

Rene Bakx’s picture

I tried that system info hook to, but the problem was that the damage was already done when that hook
is executed. Changing it on the hook level implies that I need to rebuild the entire tree of themes available just to reset two
flags back in the original state. This causes a lot of duplicated code and non needed overhead compared to the patch suggested.

But perhaps you can explain me how to get this tested, as this bug prevents most themers to actually play with, in my case Twig in D7. I can add this patch to the drush make file for the installation profile i wrote if that would speed to
process? Because as far as I could test yesterday with extending both mothership and omega with a twig based theme this little if worked like a charm.

To me it was major because it renders the child theme feature rather useless.

catch’s picture

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.

Rene Bakx’s picture

something like this you mean?

class ThemeExtendTest extends DrupalWebTestCase {
  protected $profile = 'testing';

  public static function getInfo() {
    return array(
      'name' => 'Theme Engine',
      'description' => 'Test child theme engine inherentance',
      'group' => 'Theme',
    );
  }

  function setUp() {
    parent::setUp('theme_test');
  }

  function testBaseTheme() {
    theme_enable(array('test_theme'));
    $themeInfo = list_themes(true);
    $testTheme = $themeInfo['test_theme'];
    $this->assertEqual($testTheme->owner, 'themes/engines/phptemplate/phptemplate.engine', t('Check for php-template as engine'));
  }

  function testChildTheme() {
    theme_enable(array('test_twig_theme'));
    $themeInfo = list_themes(true);
    $testTheme = $themeInfo['test_twig_theme'];
    $this->assertEqual($testTheme->owner, 'themes/engines/twig/twig.engine', t('Check for twig as engine in registry'));
  }
}

because that works if i create a second theme that inherent the test_theme if I add the extra theme in the theme_test.module.

function theme_test_system_theme_info() {
  $themes['test_theme'] = drupal_get_path('module', 'theme_test') . '/themes/test_theme/test_theme.info';
  $themes['test_twig_theme'] = drupal_get_path('module', 'theme_test') . '/themes/test_twig_theme/test_twig_theme.info';
  return $themes;
}

The only problem seems to be, that the test-suite isn't looping trough the bootstrap in the same way a normal request would because no external engines are loaded and therefor the twig_init() is never called, which results that the template.php of a twig theme is also never loaded.

Of course I could expand the test in forcing the engine to be included in some way, but that would be as useful as hardcoding every test not to fail. I feel a little lost here, cause theme engines are somewhat of a second class citizen in Drupal 7. I know for a fact that my patch works. But if the above test is to 'simple' to be a true test, I have no clue what so ever how to make a more compiling test to convince the rest of the world that my patch get's the job done.

Rene Bakx’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs review » Needs work

Missing an updated patch.

Rene Bakx’s picture

the update patch above is still valid?

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -needs backport to D7

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

Status: Needs review » Needs work
Issue tags: +Needs tests, +needs backport to D7

The last submitted patch, fix_theme_inheritance_1545964.patch, failed testing.

Rene Bakx’s picture

ah, i guess this patch needs to be rewriten against the current state of d8, however that seems a bit useless to me. The patch was created to allow twig themes extend a phptemplate in D7. D8 currently comes with twig ;)

tim.plunkett’s picture

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

Rene Bakx’s picture

Not sure if this is fixed with D8 and Twig in core now, but the problem still excists for D7 installations. How to proceed?

johnvsc’s picture

Was this ever backported to Drupal 7?

pzula’s picture

Status: Needs work » Postponed
Issue tags: +revisit before beta

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 ;)

catch’s picture

Issue summary: View changes
Status: Postponed » Active
Issue tags: -revisit before beta

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

Kars-T’s picture

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().

Kars-T’s picture

FileSize
892 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,508 pass(es). View

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

Kars-T’s picture

Status: Active » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 21: D7-theme_engine_inheritance-1545964-20-D7.patch, failed testing.

Rene Bakx’s picture

That is correct, only D7 has this issue. A backport is even impossible, because of structural changes in the theme system between D7 and D8.

Kars-T’s picture

Version: 8.0.x-dev » 7.x-dev
Issue tags: -needs backport to D7

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

Status: Needs work » Needs review
cilefen’s picture

Title: Don't copy over the owner and engine of a theme if the child theme uses a different engine then the base theme » Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme
Issue summary: View changes
Rene Bakx’s picture

I will test this later next month, kinda busy @work right now. But it would be awesome if we can get this patch in the next D7 release :)

markcarver’s picture

Version: 7.x-dev » 8.0.x-dev
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll, +needs backport to D7
Related issues: +#2512368: Support Twig engine in 7.x

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.

OnkelTem’s picture

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.

madhavvyas’s picture

Issue tags: -Needs reroll

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

pounard’s picture

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.
pounard’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: 1545964-32-engine_inheritance_fixed.patch, failed testing.

pounard’s picture

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.

pounard’s picture

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

The last submitted patch, 32: 1545964-32-engine_inheritance_fixed.patch, failed testing.

pounard’s picture

Status: Needs work » Needs review
FileSize
9.95 KB

Another patch, hope it helps:

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

Status: Needs review » Reviewed & tested by the community

RTBC - Looks great to me.

rcaracaus’s picture

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.

pounard’s picture

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...

rcaracaus’s picture

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

pounard’s picture

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.

Rene Bakx’s picture

Fabian gained committing powers for Drupal 7 if i'am right, so my best guess is that he is the one that could get it committed to core ;)

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work

#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.

Rene Bakx’s picture

Testing is indeed a very good point, yet I must admit I have no clue where to start this test. I think one way is to create a few mock arrays simulating the theme.info.

1. Normal php-template based theme.info
2. Normal other engine based theme.info
3. PHP template main theme, with other engine subtheme.

Not sure if the other way around is also something that could happen, and needs to be tested?

Edit :

Wondering how the test would pass on D.O, we can test with the above mentioned mock array, but the real theme engine and theme loading part will always fail because obviously TFD7 is not a part of core and requires a non drupal lib (Twig itself) and a Twig based theme,

Fabianx’s picture

#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'.

David_Rothstein’s picture

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?

Fabianx’s picture

Version: 7.x-dev » 8.2.x-dev

#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.

Fabianx’s picture

Version: 8.2.x-dev » 8.1.x-dev
pounard’s picture

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.

Fabianx’s picture

Version: 8.1.x-dev » 7.x-dev

#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.

Rene Bakx’s picture

But to create that test, does that mean the nyan cat needs to be ported to D7 first? Or can that engine be a part of the test itself?

Porting an engine for the sake of test sound a bit contra productive to me, because it would imply that the port of the engine needs tests as well before the engine can be committed to D7 or D7 test repository. Sounds like an awful amount of test inception work just for this rather simple patch.

Fabianx’s picture

#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.

pounard’s picture

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

Fabianx’s picture

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.

pounard’s picture

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

Fabianx’s picture

#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).

pounard’s picture

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

Pol’s picture

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

rcaracaus’s picture

Yea, would love to see this in

Pol’s picture

@pounard: any new status on this ?

pounard’s picture

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

Pol’s picture

Status: Needs work » Needs review
FileSize
10.15 KB
951 bytes

See next comment.

Status: Needs review » Needs work

The last submitted patch, 64: interdiff-64.diff, failed testing.

The last submitted patch, 64: issue-1545964-64.patch, failed testing.

Pol’s picture

Pol’s picture

Status: Needs review » Needs work

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

The last submitted patch, 67: issue-1545964-65.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
10.73 KB

Deleted.

Pol’s picture

FileSize
1.92 KB
10.75 KB

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.

The last submitted patch, 70: issue-1545964-69.patch, failed testing.

pounard’s picture

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?

Pol’s picture

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.

Pol’s picture

FileSize
2.94 KB

Dear @FabianX,

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

Thanks!

Pol’s picture

FileSize
19.61 KB

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.

Status: Needs review » Needs work

The last submitted patch, 76: 1545964-76-drupal7.patch, failed testing.

pounard’s picture

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

Pol’s picture

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.

Pol’s picture

Status: Needs work » Needs review
pounard’s picture

This looks RTBC to me.

Pol’s picture

It's not green yet :)

pounard’s picture

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

Status: Needs review » Needs work

The last submitted patch, 79: 1545964-79-drupal7.patch, failed testing.

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
688 bytes
22.05 KB

Little typo in template override test

Fabianx’s picture

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!

SebCorbin’s picture

Status: Needs review » Needs work

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

pounard’s picture

@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 :)

pounard’s picture

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.

Pol’s picture

Status: Needs work » Needs review
FileSize
18.08 KB

Hi,

Updated patch with green tests.

Thanks @sebcorbin & @pounard for finding the issue.

Status: Needs review » Needs work

The last submitted patch, 90: 1545964-90-drupal7.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
21.48 KB

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

SebCorbin’s picture

@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.

pounard’s picture

Status: Needs review » Reviewed & tested by the community

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.

Fabianx’s picture

Title: Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme » [PP-1] Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme
Status: Reviewed & tested by the community » Postponed

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.

Pol’s picture

Status: Postponed » Active
FileSize
16.14 KB

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.

pounard’s picture

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.

Fabianx’s picture

+++ 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.

pounard’s picture

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

pounard’s picture

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.

pounard’s picture

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).

pounard’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 102: 1545964-102-theme_engine_inheritance-FULL.patch, failed testing.

Pol’s picture

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:

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

Pol’s picture

Title: [PP-1] Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme » [PP-2] Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme
Rene Bakx’s picture

Awesome work everybody :) Looking forward to closing this one after 4 years :)

pounard’s picture

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

Fabianx’s picture

Title: [PP-2] Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme » Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme
Status: Needs work » Needs review

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.

The last submitted patch, 96: 1545964-96.patch, failed testing.

The last submitted patch, 101: 1545964-101-theme_engine_inheritance.patch, failed testing.

pounard’s picture

The last submitted patch, 101: 1545964-101-theme_engine_inheritance.patch, failed testing.

Pol’s picture

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

Status: Needs review » Needs work

The last submitted patch, 113: 1545964.patch, failed testing.

The last submitted patch, 102: 1545964-102-theme_engine_inheritance-FULL.patch, failed testing.

The last submitted patch, 113: 1545964.patch, failed testing.

pounard’s picture

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

Fabianx’s picture

+++ 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?

pounard’s picture

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.

Pol’s picture

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.

Pol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 120: 1545964-tests-only.patch, failed testing.

pounard’s picture

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.

The last submitted patch, 120: 1545964-tests-only.patch, failed testing.

The last submitted patch, 101: 1545964-101-theme_engine_inheritance.patch, failed testing.

Fabianx’s picture

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.

pounard’s picture

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.

pounard’s picture

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.

pounard’s picture

Status: Needs work » Needs review
FileSize
21.32 KB

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

Fabianx’s picture

Assigned: pounard » Fabianx

Thanks for the additonal tests, pounard!

pounard’s picture

Assigned: Fabianx » 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.

Pol’s picture

Assigned: pounard » Unassigned
Status: Needs review » Reviewed & tested by the community
pfrenssen’s picture

pounard’s picture

So, how is it going ?

Fabianx’s picture

Assigned: Unassigned » Fabianx

Assigning back to myself for review.

pounard’s picture

Is this still stalling ?

Rene Bakx’s picture

Perhaps tag this as Drupal 7.60 target? Would be nice if this patch will be available together with https://www.drupal.org/node/2825396 so we can release the TFD7 module based version to replace the current theme_engine release.

Fabianx’s picture

Issue tags: +Drupal 7.60 target

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

Pol’s picture

Hi there,

Any news on this ?

Thanks.

Rene Bakx’s picture

bit pissed, https://www.drupal.org/node/2826480 is useless without this patch as well :(

pounard’s picture

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

Delphine Lepers’s picture

Any hope to get this soon?

pounard’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
22.56 KB

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.

Status: Needs review » Needs work

The last submitted patch, 143: 1545964-143-theme_engine_inheritance.patch, failed testing.

pounard’s picture

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.

pounard’s picture

Status: Needs work » Needs review
Pol’s picture

Status: Needs review » Reviewed & tested by the community

All good for me.

pounard’s picture

Status: Reviewed & tested by the community » Needs work

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

pounard’s picture

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 :)

Pol’s picture

Wow nice!

Can't wait to test!

Thanks!

Status: Needs review » Needs work

The last submitted patch, 149: 1545964-149-theme_engine_inheritance.patch, failed testing.

pounard’s picture

Status: Needs work » Needs review

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.

pounard’s picture

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

Munavijayalakshmi’s picture

Status: Needs review » Needs work
  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 .

pounard’s picture

Seriously?

Munavijayalakshmi’s picture

Status: Needs work » Needs review
FileSize
24.82 KB
627 bytes
pounard’s picture

Thank you :)

pounard’s picture

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 :)

pounard’s picture

One more site working with it.

Pol’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

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:

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".

Fabianx’s picture

Assigned: Fabianx » Unassigned
pounard’s picture

@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.

simon.mellerin’s picture

Hi,

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

dan2k3k4’s picture

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

Wondering what patch file should I be using?