Add template name guesser service #78

Merged
merged 1 commit into from Dec 13, 2011

3 participants

@lenar

This change would help @schmittjoh's override of template name guessing in JMSDiExtraBundle to be implemented in a way that does not disrupt FOSRestBundle as described in schmittjoh/JMSDiExtraBundle#14.

@lenar

I implemented your suggestions. And added the changes to existing files I somehow managed to not-commit before.

@stof stof commented on the diff Nov 30, 2011
Templating/TemplateGuesser.php
+
+ /**
+ * Guesses and returns the template name to render based on the controller
+ * and action names.
+ *
+ * @param array $controller An array storing the controller object and action method
+ * @param Request $request A Request instance
+ * @param string $engine
+ * @return TemplateReference template reference
+ * @throws \InvalidArgumentException
+ */
+ public function guessTemplateName($controller, Request $request, $engine = 'twig')
+ {
+ if (!preg_match('/Controller\\\(.+)Controller$/', get_class($controller[0]), $matchController)) {
+ throw new \InvalidArgumentException(sprintf('The "%s" class does not look like a controller class (it must be in a "Controller" sub-namespace and the class name must end with "Controller")', get_class($controller[0])));
+
@stof
stof added a note Nov 30, 2011

you have an extra blank line here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof

This template guesser is used nowhere so it seems like part of your patch is missing

@stof stof commented on the diff Nov 30, 2011
Templating/TemplateGuesser.php
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class TemplateGuesser
+{
+ /**
+ * @var Symfony\Component\HttpKernel\KernelInterface
+ */
+ protected $kernel;
+
+ /**
+ * Constructor.
+ *
+ * @param ContainerInterface $container The service container instance
+ */
+ public function __construct(KernelInterface $kernel)
@stof
stof added a note Nov 30, 2011

you should inject only the kernel instead of the whole container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lenar

@stof, @henrikbjorn: Sorry I messed up the commits multiple times. It seems my mind has been taken over by the "demons" of hg.

I've implemented the changes suggested so far and added the changes that were missing. Made a clean push to branch.

Really, sorry for the inconveniences.

@henrikbjorn henrikbjorn commented on the diff Nov 30, 2011
EventListener/TemplateListener.php
@@ -60,7 +58,8 @@ class TemplateListener
}
if (!$configuration->getTemplate()) {
- $configuration->setTemplate($this->guessTemplateName($controller, $request, $configuration->getEngine()));
+ $guesser = $this->container->get('sensio_framework_extra.view.guesser');

Change the event listener to inject the guesser and remove the Container from its constructor.

@lenar
lenar added a note Nov 30, 2011

This would break the BC for FOSRestBundle (and any other bundle extending the class). Is it ok?

@lenar
lenar added a note Nov 30, 2011

Or should I use method injection? And there is the need for templating service too int his class...

i would say it is okay for the master branch.

Inject the Templating dependency and the Guesser would mean a more cleaner class and easier to test :)

@stof
stof added a note Nov 30, 2011

It is better to use constructor injection than setter injection here as they are mandatory dependencies. The class is in a broken state if it does not have the dependencies.

Listeners should be lazy-loading where possible. Thus, I think the usage is fine here.

To make it a bit cleaner you could move the container call to a dedicated method, but I don't think that's really necessary here.

@lenar
lenar added a note Nov 30, 2011

Ok, now I'm at loss.

What should I do:

1) keep it as it is currently (guesser instance created only when actually needed)
2) change to use service injection for both templating and guesser
3) inject both templating and container and still get guesser from container when it's really needed

I would prefer 1, but I'm ok with 2 too. 3 seems to be too inconsistent to my tastes. Any (final) opinions?

@stof
stof added a note Nov 30, 2011

@fabpot thoughts ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@schmittjoh schmittjoh referenced this pull request in FriendsOfSymfony/FOSRestBundle Dec 10, 2011
Closed

Consider supporting JMSSecurityExtraBundle #156

@fabpot fabpot added a commit that referenced this pull request Dec 13, 2011
@fabpot fabpot merged branch lenar/template-guesser (PR #78)
Commits
-------

929e093 Add template name guesser service (with suggested changes)

Discussion
----------

Add template name guesser service

This change would help @schmittjoh's override of template name guessing in [JMSDiExtraBundle](/schmittjoh/JMSDiExtraBundle)  to be implemented in a way that does not disrupt [FOSRestBundle](/FriendsOfSymfony/FOSRestBundle) as described in schmittjoh/JMSDiExtraBundle#14.

---------------------------------------------------------------------------

by lenar at 2011/11/30 05:01:44 -0800

I implemented your suggestions. And added the changes to existing files I somehow managed to not-commit before.

---------------------------------------------------------------------------

by stof at 2011/11/30 05:06:57 -0800

This template guesser is used nowhere so it seems like part of your patch is missing

---------------------------------------------------------------------------

by lenar at 2011/11/30 05:13:43 -0800

@stof, @henrikbjorn: Sorry I messed up the commits multiple times. It seems my mind has been taken over by the "demons" of hg.

I've implemented the changes suggested so far and added the changes that were missing. Made a clean push to branch.

Really, sorry for the inconveniences.
041c3f4
@fabpot fabpot merged commit 929e093 into sensiolabs:master Dec 13, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment