[2.3] [FrameworkBundle] [Templating] added Stopwatch support to the PHP engine #6836

Merged
merged 1 commit into from Mar 23, 2013

Conversation

Projects
None yet
3 participants
Contributor

bgarret commented Jan 22, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

I did not include tests nor documentation because there weren't any for TimedTwigEngine (I took it as an example). If I'm mistaken and they are needed, I'll gladly write them.

@stof stof commented on the diff Jan 22, 2013

...workBundle/DependencyInjection/FrameworkExtension.php
@@ -357,6 +357,9 @@ private function registerTemplatingConfiguration(array $config, $ide, ContainerB
if ($container->getParameter('kernel.debug')) {
$loader->load('templating_debug.xml');
+
+ $container->setDefinition('templating.engine.php', $container->findDefinition('debug.templating.engine.php'));
+ $container->setAlias('debug.templating.engine.php', 'templating.engine.php');
@stof

stof Jan 22, 2013

Member

I would use the alias the other way, which seems more logical: aliasing debug.templating.engine.php as templating.engine.php intead of replacing templating.engine.php with debug.templating.engine.php and then aliasing it back as debug.templating.engine.php

@bgarret

bgarret Jan 22, 2013

Contributor

Thanks for the review. I'm guilty of a bit of cargo-culting here, it was done the same way in the twig bundle.

@bgarret

bgarret Jan 25, 2013

Contributor

I actually left these as-is, because the helpers don't get loaded with a single alias (I'm getting the following error on a test app : The helper "slots" is not defined.) . I'd like to create a failing test for this, but I don't even know where to start as the failure is related to the aliasing order.

@stof stof commented on an outdated diff Jan 22, 2013

...e/FrameworkBundle/Templating/Debug/TimedPhpEngine.php
@@ -0,0 +1,59 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Templating\Debug;
@stof

stof Jan 22, 2013

Member

I don't think you need the Debug subnamespace here

Contributor

bgarret commented Jan 22, 2013

@stof I updated the pull request following your review, anything else I should do?

bgarret reopened this Jan 25, 2013

@fabpot fabpot commented on the diff Mar 23, 2013

...FrameworkBundle/Resources/config/templating_debug.xml
@@ -13,5 +14,12 @@
<tag name="monolog.logger" channel="templating" />
<argument type="service" id="logger" on-invalid="null" />
</service>
+ <service id="debug.templating.engine.php" class="%debug.templating.engine.php.class%" public="false">
+ <argument type="service" id="templating.name_parser" />
+ <argument type="service" id="service_container" />
+ <argument type="service" id="templating.loader" />
+ <argument type="service" id="debug.stopwatch" />
@fabpot

fabpot Mar 23, 2013

Owner

I would put the stopwatch as the last argument here.

Owner

fabpot commented Mar 23, 2013

Can you add a note in the bundle Changelog as well?

@fabpot fabpot added a commit that referenced this pull request Mar 23, 2013

@fabpot fabpot merged branch bgarret/timed-php-engine (PR #6836)
This PR was merged into the master branch.

Discussion
----------

[2.3] [FrameworkBundle] [Templating] added Stopwatch support to the PHP engine

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

I did not include tests nor documentation because there weren't any for TimedTwigEngine (I took it as an example). If I'm mistaken and they are needed, I'll gladly write them.

Commits
-------

3c3d34d [FrameworkBundle] [Templating] added Stopwatch support to the PHP engine
57a0f1b

fabpot closed this Mar 23, 2013

@fabpot fabpot merged commit 3c3d34d into symfony:master Mar 23, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment