I understand that alternative engines to PHPTemplate are rare (like unicorns I'd say), but I've been working on one that parses haml template using the MtHaml library to good results.

Doing so, I've uncover what I believe is a bug in theme.inc under the drupal_find_theme_templates function that prevents a template suggestions to work as expected if your new template engines doesn't use a .tpl.[extension] pattern.

Using PHPtemplate, the expected behaviour if your created a template called for example "page--user--login.tpl.php" would be replacing "page.tpl.php" for the /user/login path.

In my theme engine, extensions were only .haml

Then, creating a "page--user--login.haml" template would not work. Fiddling with my engine and making it use .tpl.haml syntax fixed it.

So then I went digging in theme.inc and found this around line 1265:

$file = substr($match, 0, strpos($match, '.'));

This basically did strip the file name when the match had no .(dot) somewhere in the template filename.

The fix can be done based on the check done at line 1236:

if (($pos = strpos($template, '.')) !== FALSE) {
  $template = substr($template, 0, $pos);
 }

I'll commit a patch next (need to figure out proper patch procedure...)

Files: 
CommentFileSizeAuthor
#1 core-theme-engine-1678808-1.patch1.01 KBAntoine Lafontaine
PASSED: [[SimpleTest]]: [MySQL] 39,271 pass(es). View

Comments

Antoine Lafontaine’s picture

FileSize
1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 39,271 pass(es). View

Here is the patch.

It's my first core patch, so please advise if things can be improved.

tlattimore’s picture

Status: Active » Needs review

Marking as needs review so the bot will run its tests and others will know to take a look at it.

Fabianx’s picture

Version: 7.x-dev » 8.x-dev
Priority: Minor » Normal
Status: Needs review » Needs work
Issue tags: +Needs tests, +Twig

Marking for .twig. Unless we switch to .html.twig this might be relevant.

This will need tests as its a fixing of a core bug.

Fabianx’s picture

This is indeed a problem and will be merged as part of #1696786: Integrate Twig into core: Implementation issue, because there we can provide tests for this.

Fabianx’s picture

Status: Needs work » Closed (duplicate)
Antoine Lafontaine’s picture

Great! Kind of get a warm feeling knowing I had a tiny contribution going in D8 core :)

jwilson3’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (duplicate) » Patch (to be ported)

Twig wont be backported to Drupal 7, but this part of that patch does seem like a candidate for backport to D7. Thoughts?

Antoine Lafontaine’s picture

@jwilson3

This is a patch against D7 and it does pass the automated tests, so I guess it could be committed to core if there's ever another bug fix release.

This is a very simple check already done in the theme engine that, somehow wasn't being done later down in the code.

It has no ties whatsoever with the inclusion of Twig or not (in D7 or 8).

Just glad it was merge and that it will, hopefully, be part of core if it doesn't get committed to D7.

jwilson3’s picture

Ah, there is the rub. It passes the automated tests, because there isn't a test to actually verify its supposed to work. Probably to get a backport, you;d need to devise a test, then prove that without your patch the test fails, and with the patch the test passes.

Fabianx’s picture

Yes, the patch could be ported from the Twig test.

Lets wait for Twig to get in, then backport the test ...

In they way I have written it, the template would need to just ship a node--2.ext and it would fail now, but succeed after the patch ...

The test is called:

testThemeTemplates or so (if anyone wants to port it) ...

Fabianx’s picture

The other instance of a hardcoded .tpl should also be fixed:

See: http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/fd40fc0