-
Notifications
You must be signed in to change notification settings - Fork 77
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add Twig functions for cmf_document_path and cmf_document_url #240
base: 3.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<container xmlns="http://symfony.com/schema/dic/services" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> | ||
|
||
<parameters> | ||
<parameter key="cmf_routing.twig_extension.class">Symfony\Cmf\Bundle\CoreBundle\Twig\Extension</parameter> | ||
<parameter key="cmf_routing.twig_extension.route_not_found_log_level">400</parameter> | ||
</parameters> | ||
|
||
<services> | ||
<service id="cmf_routing.twig_extension" class="%cmf_routing.twig_extension.class%"> | ||
<argument type="service" id="router" /> | ||
<argument type="service" id="logger" on-invalid="ignore" /> | ||
<argument>%cmf_routing.twig_extension.route_not_found_log_level%</argument> | ||
<tag name="twig.extension" /> | ||
</service> | ||
</services> | ||
</container> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony CMF package. | ||
* | ||
* (c) 2011-2014 Symfony CMF | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
|
||
namespace Symfony\Cmf\Bundle\RoutingBundle\Tests\Twig\Extension; | ||
|
||
use Symfony\Cmf\Component\Routing\Test\CmfUnitTestCase; | ||
use Symfony\Component\Routing\Exception\RouteNotFoundException; | ||
use Symfony\Cmf\Bundle\RoutingBundle\Twig\Extension\RoutingExtension; | ||
|
||
class RoutingExtensionTest extends CmfUnitTestCase | ||
{ | ||
protected $generator; | ||
|
||
public function setUp() | ||
{ | ||
$this->notFoundGenerator = $this->buildMock('Symfony\\Component\\Routing\\Generator\\UrlGeneratorInterface'); | ||
$this->notFoundGenerator->expects($this->any()) | ||
->method('generate') | ||
->will($this->throwException(new RouteNotFoundException)); | ||
} | ||
|
||
public function testCmfDocumentPathLogsRouteNotFoundException() | ||
{ | ||
$logger = $this->buildMock('Psr\Log\LoggerInterface'); | ||
|
||
$logger->expects($this->once()) | ||
->method('log'); | ||
|
||
$extension = new RoutingExtension($this->notFoundGenerator, $logger); | ||
|
||
$extension->getDocumentPath('/test/bad/path'); | ||
} | ||
|
||
public function testCmfDocumentUrlLogsRouteNotFoundException() | ||
{ | ||
$logger = $this->buildMock('Psr\Log\LoggerInterface'); | ||
|
||
$logger->expects($this->once()) | ||
->method('log'); | ||
|
||
$extension = new RoutingExtension($this->notFoundGenerator, $logger); | ||
|
||
$extension->getDocumentUrl('/test/bad/path'); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony CMF package. | ||
* | ||
* (c) 2011-2014 Symfony CMF | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
|
||
namespace Symfony\Cmf\Bundle\RoutingBundle\Twig\Extension; | ||
|
||
use Psr\Log\LoggerInterface; | ||
use Symfony\Component\Routing\Exception\RouteNotFoundException; | ||
use Symfony\Component\Routing\Generator\UrlGeneratorInterface; | ||
|
||
class RoutingExtension extends \Twig_Extension | ||
{ | ||
/** | ||
* @var UrlGeneratorInterface | ||
*/ | ||
protected $generator; | ||
|
||
/** | ||
* @var LoggerInterface | ||
*/ | ||
protected $logger; | ||
|
||
/** | ||
* @var mixed | ||
*/ | ||
protected $routeNotFoundLogLevel; | ||
|
||
/** | ||
* @param UrlGeneratorInterface $generator | ||
*/ | ||
public function __construct(UrlGeneratorInterface $generator, LoggerInterface $logger = null, $routeNotFoundLogLevel = 300) | ||
{ | ||
$this->generator = $generator; | ||
$this->logger = $logger; | ||
$this->routeNotFoundLogLevel = $routeNotFoundLogLevel; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
* TODO: Should Symfony\Bridge\Twig\Extension\RoutingExtension::isUrlGenerationSafe should be used/duplicated? | ||
*/ | ||
public function getFunctions() | ||
{ | ||
$functions = array( | ||
new \Twig_SimpleFunction('cmf_document_path', array($this, 'getDocumentPath')), | ||
new \Twig_SimpleFunction('cmf_document_url', array($this, 'getDocumentUrl')), | ||
); | ||
|
||
return $functions; | ||
} | ||
|
||
public function getDocumentPath($id, array $parameters = array(), $relative = false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering if we should add a boolean argument $suppressExceptions that defaults to true. That way you can set it to false and have exceptions bubble up. Either this or we could allow you to set this via constructor injection to this extension to let users turn on/off the exception swallowing? |
||
{ | ||
try { | ||
return $this->generator->generate($id, $parameters, $relative ? UrlGeneratorInterface::RELATIVE_PATH : UrlGeneratorInterface::ABSOLUTE_PATH); | ||
} catch (RouteNotFoundException $e) { | ||
$this->handleRouteNotFound($id, $parameters, $e); | ||
} | ||
} | ||
|
||
public function getDocumentUrl($id, array $parameters = array(), $schemeRelative = false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both of these functions duplicate the existing symfony path/url functionality and we only add the logging behavior. We could have this class extend the symfony twig routing extension and just call those functions but it wouldn't gain us much except perhaps the ability to utilize the existing isUrlGenerationSafe method |
||
{ | ||
try { | ||
return $this->generator->generate($id, $parameters, $schemeRelative ? UrlGeneratorInterface::NETWORK_PATH : UrlGeneratorInterface::ABSOLUTE_URL); | ||
} catch (RouteNotFoundException $e) { | ||
$this->handleRouteNotFound($id, $parameters, $e); | ||
} | ||
} | ||
|
||
/** | ||
* Handle a RouteNotFound exception | ||
* | ||
* @param string $id The document id (/cms/routes/route1) | ||
* @param array $parameters Route parameters | ||
*/ | ||
protected function handleRouteNotFound($id, array $parameters, RouteNotFoundException $exception) | ||
{ | ||
if (!$this->logger) { | ||
return false; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any other info needed here? |
||
$this->logger->log( | ||
$this->routeNotFoundLogLevel, | ||
'Route Not Found: '.$id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe something like "content route not found"? |
||
array( | ||
'id' => $id, | ||
'parameters' => $parameters, | ||
'exception' => $exception->getMessage() | ||
) | ||
); | ||
} | ||
|
||
public function getName() | ||
{ | ||
return 'cmf_routing'; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbu Thoughts? I don't fully grok the isUrlGenerationSafe code in the main twig extension, seems to be concerned with skipping escaping html characters in the url under certain circumstances for performance reasons so I dont think its critical here. In order to leverage that code without copy pasting this class would have to extend it it seems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would extend/instantiate https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/RoutingExtension.php directly in this class and forward getObjectPath to it with a try-catch around the getPath call. then we are sure we do exactly the same and avoid any duplication.