Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

Support for setting the redirect content as a content document #163

Open
wants to merge 1 commit into
base: master
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
33 changes: 31 additions & 2 deletions Adapter/PhpcrOdmAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,30 @@ class PhpcrOdmAdapter implements AdapterInterface
{
const TAG_NO_MULTILANG = 'no-multilang';

/**
* Set the redirect target to the new auto-routes content document
*/
const REDIRECT_CONTENT = 'content';

/**
* Set the redirect target to the new auto-route itself.
*/
const REDIRECT_ROUTE = 'route';

protected $dm;
protected $baseRoutePath;
protected $autoRouteFqcn;
protected $redirectTarget = self::REDIRECT_ROUTE;

/**
* @param DocumentManager $dm
* @param string $routeBasePath Route path for all routes
* @param string $autoRouteFqcn The FQCN of the AutoRoute document to use
* @param string $redirectTarget The target type to use when
* leaving a redirect route, either self::REDIRECT_ROUTE, or
* self::REDIRECT_CONTENT/
*/
public function __construct(DocumentManager $dm, $routeBasePath, $autoRouteFqcn = 'Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute')
public function __construct(DocumentManager $dm, $routeBasePath, $autoRouteFqcn = 'Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute', $redirectTarget = self::REDIRECT_ROUTE)
{
$this->dm = $dm;
$this->baseRoutePath = $routeBasePath;
Expand All @@ -51,6 +65,7 @@ public function __construct(DocumentManager $dm, $routeBasePath, $autoRouteFqcn
}

$this->autoRouteFqcn = $autoRouteFqcn;
$this->redirectTarget = $redirectTarget;
}

/**
Expand Down Expand Up @@ -154,7 +169,21 @@ public function createAutoRoute(UriContext $uriContext, $contentDocument, $autoR
*/
public function createRedirectRoute(AutoRouteInterface $referringAutoRoute, AutoRouteInterface $newRoute)
{
$referringAutoRoute->setRedirectTarget($newRoute);
switch ($this->redirectTarget) {
case self::REDIRECT_CONTENT:
$target = $newRoute->getContent();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imho, we should make redirect to content the default, but fall back to the route if there is no content (which is a valid situation). i don't think we need the configuration options with constants, if there is a content of a route and it implements RouteReferrersReadInterface, we should take that over the route. but only in that case. the content could be not route aware, or there can be routes with no content object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing that concerns me is the change of behavior - the option is there to preserve the existing behavior, if we now set the redirect target as the content, and the user is handling the redirect as if it were always a Route object, then it's a BC break ?

break;
case self::REDIRECT_ROUTE:
$target = $newRoute;
break;
default:
throw new \RuntimeException(sprintf(
'Unknown redirect target type "%s"',
$this->redirectTarget
));
}

$referringAutoRoute->setRedirectTarget($target);
$referringAutoRoute->setType(AutoRouteInterface::TYPE_REDIRECT);
}

Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
Changelog
=========

dev-master
----------

* Support for redirecting to content instead of other routes.

1.0.0
-----

Expand Down
2 changes: 1 addition & 1 deletion Model/AutoRoute.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function setType($type)
$this->setDefault('type', $type);
}

public function setRedirectTarget(AutoRouteInterface $redirectRoute)
public function setRedirectTarget($redirectRoute)
{
$this->redirectRoute = $redirectRoute;
}
Expand Down
6 changes: 3 additions & 3 deletions Tests/Functional/EventListener/AutoRouteListenerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ public function provideLeaveRedirect()
/**
* @dataProvider provideLeaveRedirect
*/
public function testLeaveRedirect($data, $updatedData, $expectedRedirectRoutePaths, $expectedAutoRoutePaths)
public function testLeaveRedirect($data, $updatedData, $expectedRedirectRoutePaths, $expectedPaths)
{
$article = new SeoArticleMultilang;
$article->title = 'Hai';
Expand Down Expand Up @@ -380,9 +380,9 @@ public function testLeaveRedirect($data, $updatedData, $expectedRedirectRoutePat
$this->assertEquals(AutoRouteInterface::TYPE_REDIRECT, $redirectRoute->getDefault('type'));
}

foreach ($expectedAutoRoutePaths as $newPath) {
foreach ($expectedPaths as $newPath) {
$autoRoute = $this->getDm()->find(null, $newPath);
$this->assertNotNull($autoRoute, 'Autoroute exists for: ' . $newPath);
$this->assertNotNull($autoRoute, 'Redirect target for: ' . $newPath);
$this->assertEquals(AutoRouteInterface::TYPE_PRIMARY, $autoRoute->getDefault('type'));
}
}
Expand Down
56 changes: 56 additions & 0 deletions Tests/Unit/Adapter/PhpcrOdmAdapterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,60 @@ public function testFindRouteForUri()
$res = $this->adapter->findRouteForUri($uri, $this->uriContext->reveal());
$this->assertSame($expectedRoutes, $res);
}

/**
* It should set the redirect target as the content document when configured to do so.
*/
public function testCreateRedirectRouteContent()
{
$adapter = new PhpcrOdmAdapter(
$this->dm->reveal(),
$this->baseRoutePath,
'Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute',
PhpcrOdmAdapter::REDIRECT_CONTENT
);
$newRoute = $this->prophesize('Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute');
$newRoute->getContent()->willReturn($this->contentDocument);

$adapter->createRedirectRoute($this->route->reveal(), $newRoute->reveal());
$this->route->setRedirectTarget($this->contentDocument)->shouldHaveBeenCalled();
}

/**
* It should set the redirect target as route when configured to do so.
*/
public function testCreateRedirectRoute()
{
$adapter = new PhpcrOdmAdapter(
$this->dm->reveal(),
$this->baseRoutePath,
'Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute',
PhpcrOdmAdapter::REDIRECT_ROUTE
);
$newRoute = $this->prophesize('Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute');
$newRoute->getContent()->shouldNotBeCalled();

$adapter->createRedirectRoute($this->route->reveal(), $newRoute->reveal());
$this->route->setRedirectTarget($newRoute)->shouldHaveBeenCalled();
}

/**
* It should throw an exception if the redirect target type is not valid
*
* @expectedException RuntimeException
* @expectedExceptionMessage Unknown redirect target type
*/
public function testInvalidRedirectTargetType()
{
$adapter = new PhpcrOdmAdapter(
$this->dm->reveal(),
$this->baseRoutePath,
'Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute',
'foobar' // invalid
);
$newRoute = $this->prophesize('Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute');

$adapter->createRedirectRoute($this->route->reveal(), $newRoute->reveal());
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dd

}
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"prefer-stable": true,
"require": {
"php": ">=5.3.9",
"symfony-cmf/routing-auto": "~1.1",
"symfony-cmf/routing-auto": "dev-remove_typehint@dev",
"symfony-cmf/routing-bundle": "~1.2,>=1.2.0",
"symfony-cmf/core-bundle": "~1.2",
"aferrandini/urlizer": "1.0.*",
Expand Down