Skip to content
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

Open
wants to merge 3 commits into
base: 3.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions DependencyInjection/CmfRoutingExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public function load(array $configs, ContainerBuilder $container)
{
$config = $this->processConfiguration(new Configuration(), $configs);
$loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config'));
$loader->load('services.xml');

if ($config['dynamic']['enabled']) {
// load this even if no explicit enabled value but some configuration
Expand Down
20 changes: 20 additions & 0 deletions Resources/config/services.xml
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>

55 changes: 55 additions & 0 deletions Tests/Unit/Twig/Extension/RoutingExtensionTest.php
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');
}

}
105 changes: 105 additions & 0 deletions Twig/Extension/RoutingExtension.php
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?
Copy link
Member Author

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?

Copy link
Member

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.

*/
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)
Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Copy link
Member Author

Choose a reason for hiding this comment

The 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;
}

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Member

Choose a reason for hiding this comment

The 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';
}
}