Allow modules to provide both .twig and .tpl.php templates temporarily until twig is the default engine

more tagging

#0 looks good for me

This seems at least major to me.

New patch for this:

* Make theme_engine the default engine if different from default engine.
* Make it possible to fallback to default engine if no template file was found for the current theme engine

This allows to set for example:

engine = twig in a theme and it will also find a node.twig in the core/modules/node/node.twig

Example searches are:

check: node.twig
check: node.tpl.php

This is a blocker for #1696786: Integrate Twig into core: Implementation issue.

Priority back to major as per #6.

Here are some testing instructions from IRC:

tlattimore: fabianx: Cool. WIll review in a bit
[5:27pm] tlattimore: fabianx: Whats the easiest way to test this?
[5:27pm] Fabianx: tlattimore: Apply http://drupal.org/node/1696786#comment-6556998 and move node.twig to core/modules/node/node.twig
[5:28pm] tlattimore: fabianx: With what them enabled?
[5:28pm] Fabianx: tlattimore: Clear cache and see it being used.
[5:29pm] tlattimore: fabianx: So, would have to test with stark then?
[5:29pm] Fabianx: tlattimore: yes
[5:29pm] Fabianx: tlattimore: Because that one has engine = twig

I have functionally tested and reviewed the patch #7 and can confirm it works as described.

I tested as follows:
* Applied patch from #7
* Then applied patch from http://drupal.org/files/core-Integrate-twig-into-core-1696786-27_0.diff
-- This modifies the stark theme to use the twig engine and therefore call *.twig templates, this step is simply to test that the changes work correctly
* Then I MOVED (not copied) the node.twig from stark to core/modules/node/

After following these steps (and clearing the cache) the node.twig was then being called from core/modules/node/ correctly.

RTBC!

#7 looks good for me too

+++ b/core/includes/theme.inc
@@ -1039,17 +1039,35 @@ function theme($hook, $variables = array()) {
+      if (function_exists($theme_engine . '_render_template')) {
...
+      if (function_exists($extension_function)) {
+        $theme_engine_extension = $extension_function();
+      }

...that looks like a lot of additional processing for every single theme() call on a template. Any chance to cache this always static theme engine info?

If there's no obvious way and existing facility, then let's create a major follow-up issue to introduce one.

+++ b/core/includes/theme.inc
@@ -1069,11 +1087,19 @@ function theme($hook, $variables = array()) {
+        breaK;

lettercase typo

This needs tests as well.

After discussion with catch:

Fabianx: catch|afk: I think I know how to do it properly. Lets have hook_engine_info that returns extension and theme_function. Lets also have theme_rebuild_library do the lookup of the template based on all available engines and store the right engine in the theme_library. Then $info contains $info['engine'] and the code is very straightforward.
catch|afk: Fabianx: so no file_exists() on runtime then?
Fabianx: catch|afk: yep
catch|afk: Fabianx: that sounds better yeah 
Fabianx: catch|afk: That was also my biggest concern.
catch|afk: Fabianx: I'd be Ok with runtime file exists if there's a major/critical to remove it, but not if there's any chance this stays in past release.
Fabianx: catch|afk: Which tests are then needed?
catch|afk: Fabianx: hmm. One template that's .twig (or .test_theme_engine_, one that's .tpl.php, check the theme registry to make sure they're noth picked up or something?
Fabianx: catch|afk: okay
catch|afk: Fabianx: but anything is fine really if you find another way.
Fabianx: catch|afk: That sounds better. 
catch|afk: Fabianx: but that's actually testing the template discovery then Twig implementations will eventually have their own test coverage.
Fabianx: catch|afk: sure
Fabianx: catch|afk: but its specific to multiple engines ...
Fabianx: catch|afk: Is hook_engine_info (like hook_views_info etc.) still a best practice in D8 or is there another pattern used now?
catch|afk: Fabianx: I couldn't believe when I saw the twig patch that we had hook_init()
catch|afk: for theme engines.
catch|afk: 'cos wtf.
catch|afk: Fabianx: probably theme engines should be a class which define those methods, but that's completely separate to this bug fix so it's OK to add the hook I think.
catch|afk: They're not even hooks
Fabianx: catch|afk: okay
Fabianx: catch|afk: Then we leave that as followup.

This needs a different approach and tests for multiple engines.

Needs review: ONLY FOR TESTBOT

This is including a bad, bad, bad hack and hardcodes twig.

Description of changes:

   Issue #1697854: Allow several engines at the same time.
    
    * Implements hook_engine_info() instead of hook_extension().
    * Moves all template detection code to _theme_process_registry().
    * Degrades gracefully to the default engine if registry is not build.
    ** @todo: Is this necessary.
    
    * @todo: Uses a major hack to call a engine_invoke_all().
    * @todo: Has twig hard-coded for now to test.

This will allow to use any registered engines parallel at the same time.

Engine registering is the problem and does not work as a module_invoke_all is not possible and an engine_invoke_all does not exist.

As this would need a follow-up issue anyway to move engines to the plugin system, we need to do the follow-up before this issue can go in.

Will search for / open issue now.

Hmmm, without the phptemplate.engine change this will never run the new code path.

New patch including phptemplate.engine attached.

Again: ONLY FOR TESTBOT. Comments from #14 apply.

Okay, back to the drawing board:

Goals this patch wants to achieve:

* Current engine functionality works as is and is _not_ changed
* Only for modules will core support temporarily several engines (twig and phptemplate as followup)
* The engines are hardcoded and are also loaded by default - if they exist.
* Drupal implements drupal_get_theme_engines for this
* The detection of module .twig / .tpl.php files is done in _registry_rebuild like in current patch.

=> Only if its a module, will $info['template_file'] be set and only in that case will it be a different engine then the default one.

New approach with hardcoded twig as second engine only for modules.

Falls back gracefully if twig.engine is not yet available and templates are printed via phptemplate instead as verbatim ...

Explanation of patch

This is a temporary patch and will be removed before release per #1805592: Remove static double theme engine code once all module provided .tpl.php files are converted to twig and twig is default.

First of all both phptemplate.engine and twig.engine need to be loaded at all times for modules to work with both template types. phptemplate is the default, so it is always loaded.
Twig is the temporary additional engine, so it is loaded always in _drupal_theme_initialize if twig.engine exists.

Second the _theme_process_registry function is changed, to support only for type = module temporarily two theme engines and if a template could be found via file_exists both the engine and the template_file is stored as part of the registry.

Third in case of a module, the engine is taken from $info['engine'] if it exists and the engine is looked up _like_ for themes. Additionally if $info['template_file'] is set, this is used instead.

Thanks goes to chx for mentoring :-).

This is the simplest way that we could come up with to unblock TWIG in Core.

This is a simple temporary patch to facilitate current work to get Twig into core. It's not intended to be included in the final d8 release. In that regard, it looks RTBC to me.

Updated issue summary.

updated summary to include follow up.

Although I understand putting in a temporarary fix to get around Drupal's gross template engine system, but could try and make the engine system a bit less-gross in the process?

[EDIT] Was working on #304486: Theme Engines as Modules a while ago. Might be worth another look.

@sun, #11:

Yes, already without this patch there is processing going on by run-time by calling function_exists, but most of this is needed due to some other things. Function_exists seems to be not too expensive. Could be changed some time.

@Rob Loach:

I am happy to do that as part of another issue, but we don't have the time at the moment for this PATCH, because this blocks #1696786: Integrate Twig into core: Implementation issue and we need to get twig in ASAP to unblock other work that needs review, testing, etc.

I tried hard to fix the engine system while doing so, but it is very complex, would need a pleora of new tests (that are not there currently) and is totally away from the goals of the twig initiative.

What we need for TWIG to be a success is this to get commited and then more reviews on #1696786: Integrate Twig into core: Implementation issue and get that commited as well.

I am removing "needs tests" tag for this issue as we need twig.engine in to actually test this. We will provide proper tests that .twig and .tpl.php files are rendered correctly as part of #1696786: Integrate Twig into core: Implementation issue.

A manual review will find that core still finds the .tpl.php files for modules with this patch included.

Merged here: #1696786-55: Integrate Twig into core: Implementation issue

Explain issue a little better