NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

webmozart wants to merge1 commit intosymfony:masterfromwebmozart:improve-name-parser

Closed

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

webmozart wants to merge1 commit intosymfony:masterfromwebmozart:improve-name-parser

Conversation

Copy link

Contributor

webmozart commentedApr 23, 2013

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 theTemplateNameParser class. Before, the syntax could only be used in Twig's own functionality, such as theextend orinclude 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

<?phpuseSymfony\Bundle\FrameworkBundle\Templating\TemplateNameParser;require__DIR__.'/vendor/autoload.php';require__DIR__.'/app/AppKernel.php';$kernel =newAppKernel('dev',true);$kernel->boot();$parser =newTemplateNameParser($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

<?phpuseSymfony\Bundle\FrameworkBundle\Templating\TemplateNameParser;require__DIR__.'/vendor/autoload.php';require__DIR__.'/app/AppKernel.php';$kernel =newAppKernel('dev',true);$kernel->boot();$parser =newTemplateNameParser($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

<?phpuseSymfony\Bundle\FrameworkBundle\Templating\TemplateNameParser;require__DIR__.'/vendor/autoload.php';require__DIR__.'/app/AppKernel.php';$kernel =newAppKernel('dev',true);$kernel->boot();$parser =newTemplateNameParser($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.

[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

Copy link

Member

stof commentedApr 23, 2013

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

Copy link

ContributorAuthor

webmozart commentedApr 23, 2013

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

Copy link

Member

stof commentedApr 23, 2013

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

Copy link

ContributorAuthor

webmozart commentedApr 23, 2013

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

How do I currently break Twig's features?

Copy link

Member

stof commentedApr 23, 2013

If I register a namespace pathFoobar 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)

Copy link

Contributor

beberlei commentedApr 30, 2013

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.

Copy link

ContributorAuthor

webmozart commentedApr 30, 2013

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

Copy link

Contributor

beberlei commentedApr 30, 2013

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

Copy link

ContributorAuthor

webmozart commentedApr 30, 2013

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