[FrameworkBundle] Made TemplateNameParser able to parse templates in the @-notation (#5660) #7818

webmozart · 2013-04-23T13:45:41Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#6918
License	MIT
Doc PR	TODO

This PR adds the ability to handle Twig's new template name syntax (see fabpot/Twig#772 and #5660) to the TemplateNameParser class. Before, the syntax could only be used in Twig's own functionality, such as the extend or include statements. Now, the syntax can be used anywhere in a Symfony application, such as when returning from a Controller, the @Template annotation or with PHP Templating. The old syntax "bundle:controller:template" is still supported, but can be completely replaced*.

Examples:

New Syntax	Old Syntax (still supported)
`index.html.twig`	`::index.html.twig`
`Foo/bar.html.php`	`:Foo:bar.html.php`
`@AcmeDemo/test.html.twig`	`AcmeDemoBundle::test.html.twig`
`@AcmeDemo/Post/create.html.php`	`AcmeDemoBundle:Post:create.html.php`

* The support for bundle inheritance is still missing for Twig: #6918

Benchmarks

Performance benchmark:

	Old paths	New paths (leading @)	New paths (no leading @)
Before	~3.20ms	-	-
After	~3.20ms	~3.25ms	~2.90ms

Scripts:

benchmark-old.php

<?php

use Symfony\Bundle\FrameworkBundle\Templating\TemplateNameParser;

require __DIR__.'/vendor/autoload.php';
require __DIR__.'/app/AppKernel.php';

$kernel = new AppKernel('dev', true);
$kernel->boot();
$parser = new TemplateNameParser($kernel);


$time = microtime(true);

for ($i = 0; $i < 50; ++$i) {
    $parser->parse("AcmeDemoBundle:Foo:bar$i.html.twig");
}

echo "Time:   " . (microtime(true)-$time)*1000 . "ms\n";
echo "Memory: " . memory_get_peak_usage(true)/(1024*1024) . "MB\n";

benchmark-new.php

<?php

use Symfony\Bundle\FrameworkBundle\Templating\TemplateNameParser;

require __DIR__.'/vendor/autoload.php';
require __DIR__.'/app/AppKernel.php';

$kernel = new AppKernel('dev', true);
$kernel->boot();
$parser = new TemplateNameParser($kernel);


$time = microtime(true);

for ($i = 0; $i < 50; ++$i) {
    $parser->parse("@AcmeDemo/Foo/bar$i.html.twig");
}

echo "Time:   " . (microtime(true)-$time)*1000 . "ms\n";
echo "Memory: " . memory_get_peak_usage(true)/(1024*1024) . "MB\n";

benchmark-new2.php

<?php

use Symfony\Bundle\FrameworkBundle\Templating\TemplateNameParser;

require __DIR__.'/vendor/autoload.php';
require __DIR__.'/app/AppKernel.php';

$kernel = new AppKernel('dev', true);
$kernel->boot();
$parser = new TemplateNameParser($kernel);


$time = microtime(true);

for ($i = 0; $i < 50; ++$i) {
    $parser->parse("AcmeDemoBundle/Foo/bar$i.html.twig");
}

echo "Time:   " . (microtime(true)-$time)*1000 . "ms\n";
echo "Memory: " . memory_get_peak_usage(true)/(1024*1024) . "MB\n";

Remaining Concerns

Drop the "Bundle" suffix or not?

My remaining concern is about the convention to drop the bundle suffix in the new syntax. I dislike it for three reasons:

it is inconsistent with the resource import syntax (@AcmeDemo/index.html.twig vs. @AcmeDemoBundle/Resources/config/routing.yml)
the paths for templates from a bundle vs. globally overridden templates are inconsistent (@AcmeDemo/index.html.twig vs. AcmeDemoBundle/index.html.twig)
when an author creates both a library and some bundle to integrate it, he can't easily add namespaces for both of them (e.g. library "Monolog" and bundle "MonologBundle" - the namespace for the first would be @Monolog/template.html.twig but for the second as well, creating a conflict)

@fabpot stated in #5660 that an advantage of dropping the "Bundle" suffix is making the namespaces

more agnostic for when the templates are used outside of the full-stack framework

I disagree. When I load MonologBundle into my non-Symfony application, it makes sense that the namespace is "MonologBundle".

Adapt getLogicalName() or not?

The second question we have to solve is whether we change the logical template name (TemplateReference::getLogicalName()) to the new syntax as well. I left it at the old syntax for now because I don't know what the BC implications would be.

…the @-notation (symfony#5660)

stof · 2013-04-23T14:07:31Z

@bschussek I see an issue here: as you are parsing the @ notation in the TemplateNameParser rather than doing it in Twig, it does not work for template namespaces registered in the Twig loader without being bundles.
So it is either inconsistent with the loading using {{ include }} in templates or even worse breaking it (I think your change will make Twig search in a non-existant bundle instead of falling back to the namespaced path)

webmozart · 2013-04-23T14:09:44Z

@stof Yes, you're right. We have to replace the bundle check by a namespace check. But we can only do this if we also register the namespaces in the Templating component (apart from Twig).

stof · 2013-04-23T14:33:59Z

@bschussek currently, the TwigBundle FilesystemLoader is using the parser and then fallback to using the string itself as template name directly. This works well and does not break the Twig features. All we need is allowing the TwigEngine to work this way too instead of forcing to have a bundle notation IMO.

webmozart · 2013-04-23T14:57:09Z

@stof We need namespaces also for PHP templating in order to fix #6536.

How do I currently break Twig's features?

stof · 2013-04-23T16:06:56Z

If I register a namespace path Foobar and then call {{ include('@Foobar/test.html.twig') }}, your code will try to parse it as being part of the FoobarBundle. This is wrong as Twig could have a older register for the @Foobar namespace with a higher priority than the bundle folder whereas you will know load it from FoobarBundle (using the parsing as a TemplateReference instead of the Twig namespaces)

beberlei · 2013-04-30T12:53:42Z

Regarding the Bundle suffix, we tried this in early alpha some time i remember and then unilaterally wanted to move back to the Bundle suffix one alpha later.

webmozart · 2013-04-30T13:51:45Z

@beberlei I'm slow today, what's your point? :) Are you saying we should bring back the "Bundle" suffix here as well?

beberlei · 2013-04-30T14:13:28Z

@bschussek Bring back? The todo says you are evaluating to remove it here, did you already?

webmozart · 2013-04-30T14:51:52Z

Oh, this is badly named then :) The Bundle suffix is already dropped for this notation. The questions is whether we bring it back in 2.3 (now that the syntax is not that wide-spread yet) or to leave this inconsistency in at least until 3.0. Ping @fabpot

webmozart · 2013-06-15T16:39:54Z

A very interesting proposal came up in the FIG that could solve the problems mentioned here in a much more generic scope:

https://groups.google.com/forum/?fromgroups#!topic/php-fig/WMaKNNhHZJw

ahilles107 · 2013-09-02T05:55:30Z

👍 for this PR, in Sourcefabric we wait for that, tell if we can help!

webmozart · 2014-03-11T12:10:15Z

Closing this. The same functionality - but better - will be delivered by webmozart/symfony-puli-bundle at some point.

[FrameworkBundle] Made TemplateNameParser able to parse templates in …

cf3adf2

…the @-notation (symfony#5660)

webmozart mentioned this pull request Apr 23, 2013

[3.0] [Form] Move templates from FrameworkBundle to Form #6536

Closed

9 tasks

sstok mentioned this pull request Aug 30, 2013

Namespaced template names don't work when twig and php are selected as templating engine #8892

Closed

fabpot mentioned this pull request Sep 27, 2013

[2.3] [WIP][FrameworkBundle] Add support for the '@Bundle/Path/template.format.engi... #6918

Closed

5 tasks

webmozart closed this Mar 11, 2014

webmozart deleted the improve-name-parser branch March 11, 2014 12:10

sstok mentioned this pull request Nov 15, 2014

[Framework] Template name is not valid #12249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[FrameworkBundle] Made TemplateNameParser able to parse templates in the @-notation (#5660) #7818

[FrameworkBundle] Made TemplateNameParser able to parse templates in the @-notation (#5660) #7818

Uh oh!

webmozart commented Apr 23, 2013

Uh oh!

stof commented Apr 23, 2013

Uh oh!

webmozart commented Apr 23, 2013

Uh oh!

stof commented Apr 23, 2013

Uh oh!

webmozart commented Apr 23, 2013

Uh oh!

stof commented Apr 23, 2013

Uh oh!

beberlei commented Apr 30, 2013

Uh oh!

webmozart commented Apr 30, 2013

Uh oh!

beberlei commented Apr 30, 2013

Uh oh!

webmozart commented Apr 30, 2013

Uh oh!

webmozart commented Jun 15, 2013

Uh oh!

ahilles107 commented Sep 2, 2013

Uh oh!

webmozart commented Mar 11, 2014

Uh oh!

Uh oh!

Uh oh!

[FrameworkBundle] Made TemplateNameParser able to parse templates in the @-notation (#5660) #7818

[FrameworkBundle] Made TemplateNameParser able to parse templates in the @-notation (#5660) #7818

Uh oh!

Conversation

webmozart commented Apr 23, 2013

Benchmarks

Remaining Concerns

Uh oh!

stof commented Apr 23, 2013

Uh oh!

webmozart commented Apr 23, 2013

Uh oh!

stof commented Apr 23, 2013

Uh oh!

webmozart commented Apr 23, 2013

Uh oh!

stof commented Apr 23, 2013

Uh oh!

beberlei commented Apr 30, 2013

Uh oh!

webmozart commented Apr 30, 2013

Uh oh!

beberlei commented Apr 30, 2013

Uh oh!

webmozart commented Apr 30, 2013

Uh oh!

webmozart commented Jun 15, 2013

Uh oh!

ahilles107 commented Sep 2, 2013

Uh oh!

webmozart commented Mar 11, 2014

Uh oh!

Uh oh!