Enable the ContentObject provider to work with translated referrers #63
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ | |
| use Doctrine\ODM\PHPCR\DocumentManager; | ||
| use Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Phpcr\Route; | ||
| use Doctrine\Common\Collections\ArrayCollection; | ||
| use Doctrine\Common\Util\ClassUtils; | ||
| use Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute; | ||
|
|
||
| /** | ||
| * Provides path elements by determining them from | ||
|
|
@@ -65,15 +67,28 @@ public function providePath(RouteStack $routeStack) | |
|
|
||
| $object = $contentObject->$method(); | ||
|
|
||
| if (!$object) { | ||
| throw new \RuntimeException(sprintf( | ||
| 'The %s:%s method has returned an empty value "%s"', | ||
| get_class($contentObject), | ||
| $method, | ||
| var_export($object, true) | ||
| )); | ||
| } | ||
|
|
||
| $routeFilter = function ($referrer) use ($object, $context) { | ||
| if ($referrer instanceof AutoRoute && $referrer->getContent() === $object) { | ||
|
There was a problem hiding this comment. We now require an instance of There was a problem hiding this comment. you could try to rely on the LocaleListener for having locales in auto routes, no? There was a problem hiding this comment. If we did that would it mean that the URl would not work? Not a common use case, but I don't see any reason for saying its not a valid one (?) There was a problem hiding this comment. Also, the There was a problem hiding this comment. hm, yes, the locale would have to be the first part. i guess it makes sense to not rely on the listener then. and users are not expected to edit AutoRoute by hand, so its different (on normal routes, without the listener you could move your route in the tree and it would be at /de but the stored locale be "fr"...) about the pattern: there are two multilang scenarios, one where you have just one route per content and have the pattern, the other with multiple translated routes that are each for only one locale. but i guess with autoroute, supporting the pattern approach with a single route makes no sense at all (its mostly useful for pages that are routes at the same time, like simple cms, and there you need no auto routes) |
||
|
|
||
| // filter the referrering routes by locale | ||
| if ($referrer->getLocale() == $context->getLocale()) { | ||
|
There was a problem hiding this comment. Note that this does not support fallback yet, it should do before this PR is done. |
||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| }; | ||
|
|
||
| $referringRoutes = new ArrayCollection(); | ||
|
|
||
| if ($this->documentIsPersisted($object)) { | ||
| // check to see existing routes | ||
|
|
@@ -91,7 +106,7 @@ public function providePath(RouteStack $routeStack) | |
|
|
||
| if (count($routes) > 1) { | ||
| throw new \RuntimeException( | ||
| 'Found more than one referring auto route, this should not happen.' | ||
|
There was a problem hiding this comment. should this not go into a logfile, at a high level? an exception could break a running website, which is probably too much. There was a problem hiding this comment. I think it depends on if this would ever happen, and under what circumstances it would happen. It shouldn't happen I think, so I would prefer to bail rather than risk the website acting in unexpected ways. |
||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,4 +21,16 @@ | |
| */ | ||
| class AutoRoute extends Route | ||
| { | ||
| protected $locale; | ||
|
|
||
| public function getLocale() | ||
| { | ||
| return $this->locale; | ||
| } | ||
|
|
||
| public function setLocale($locale) | ||
| { | ||
| $this->locale = $locale; | ||
| } | ||
|
There was a problem hiding this comment. note taht normal routes store the locale in a route _default, and there is the LocaleListener that adjusts that according to the path. i am in the process of finding a way to make routes locale capable even if there is just one, porting simplcms things to RoutingBundle - see symfony-cmf/routing-bundle#210 i am a bit afraid that these things collide with the other model, or we end up in too many different ways to do the same thing. There was a problem hiding this comment. The There was a problem hiding this comment. that listener is meant to ensure that routes always have the right locale according to their repository path. as said above, i follow your reasoning. there is one point left: why not store _locale in the defaults, rather than have this separate field? that way, symfony would see the locale too. There was a problem hiding this comment. Yeah I guess that would make sense. Is there ever an edge risk that the There was a problem hiding this comment. if the route is at a tree root that the listener looks at, and starts do you think people should mix their own routes with an autoroute tree? There was a problem hiding this comment. I think the users shouldn't, but there is no way to stop them doing this, unless we have a system to segrate routes into zones (e.g. all children of Maybe the There was a problem hiding this comment. or we could make the listener only change a locale on a move event, and in all other cases leave the route untouched if it already has a locale (even if its not the expected one). wdyt? if you agree, can you open an issue on the RoutingBundle? can we remove the whole explict locale thing from here and use the defaults for the locale? |
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| <doctrine-mapping xmlns="http://doctrine-project.org/schemas/phpcr/doctrine-mapping" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"> | ||
|
|
||
| <document name="Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute"> | ||
| <field name="locale" type="string" nullable="true"/> | ||
| </document> | ||
|
|
||
|
|
||
| </doctrine-mapping> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| <?php | ||
|
|
||
|
|
||
| namespace Functional\EventListener; | ||
|
There was a problem hiding this comment. should be |
||
|
|
||
| use Symfony\Cmf\Bundle\RoutingAutoBundle\Tests\Resources\Document\Article; | ||
| use Symfony\Cmf\Bundle\RoutingAutoBundle\Tests\Functional\BaseTestCase; | ||
| use Symfony\Cmf\Bundle\RoutingAutoBundle\Tests\Resources\Document\Category; | ||
|
|
||
| class AutoRouteListenerMultilangTest extends BaseTestCase | ||
| { | ||
| public function provideMultilangCategory() | ||
| { | ||
| return array( | ||
| array( | ||
| array( | ||
| 'en' => 'Hello everybody!', | ||
| 'fr' => 'Bonjour le monde!', | ||
| 'de' => 'Gutentag', | ||
| 'es' => 'Hola todo el mundo', | ||
| ), | ||
| array( | ||
| 'test/auto-route/articles/en/hello-everybody', | ||
| 'test/auto-route/articles/fr/bonjour-le-monde', | ||
| 'test/auto-route/articles/de/gutentag', | ||
| 'test/auto-route/articles/es/hola-todo-el-mundo', | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider provideMultilangArticle | ||
| */ | ||
| public function testMultilangCategory($data, $expectedPaths) | ||
| { | ||
| $category = new Category(); | ||
| $category->path = '/test/category1'; | ||
| $this->getDm()->persist($category); | ||
|
|
||
| foreach (array( | ||
| 'de' => 'Meine neue Kategorie', | ||
| 'fr' => 'Ma nouvelle catégorie', | ||
| 'en' => 'My new category', | ||
| ) as $locale => $title) { | ||
| $category->title = $title; | ||
| $this->getDm()->bindTranslation($category, $locale); | ||
| } | ||
| } | ||
|
|
||
| public function provideMultilangArticle() | ||
| { | ||
| return array( | ||
| array( | ||
| array( | ||
| 'en' => 'Hello everybody!', | ||
| 'fr' => 'Bonjour le monde!', | ||
| 'de' => 'Gutentag', | ||
| 'es' => 'Hola todo el mundo', | ||
| ), | ||
| array( | ||
| 'test/auto-route/articles/en/category/hello-everybody', | ||
| 'test/auto-route/articles/fr/category/bonjour-le-monde', | ||
| 'test/auto-route/articles/de/category/gutentag', | ||
| 'test/auto-route/articles/es/category/hola-todo-el-mundo', | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider provideMultilangArticle | ||
| */ | ||
| public function testMultilangArticle($data, $expectedPaths) | ||
| { | ||
| $category = new Category(); | ||
| $category->path = '/test/category1'; | ||
| $category->title = 'category'; | ||
| $this->getDm()->persist($category); | ||
| foreach (array_keys($data) as $lang) { | ||
| $this->getDm()->bindTranslation($category, $lang); | ||
| } | ||
|
|
||
| $article = new Article; | ||
| $article->path = '/test/article-1'; | ||
| $article->categories[] = $category; | ||
|
|
||
| $this->getDm()->persist($article); | ||
|
|
||
| foreach ($data as $lang => $title) { | ||
| $article->title = $title; | ||
| $this->getDm()->bindTranslation($article, $lang); | ||
| } | ||
|
|
||
| $this->getDm()->flush(); | ||
| $this->getDm()->clear(); | ||
|
|
||
| $articleTitles = array_values($data); | ||
| foreach ($expectedPaths as $i => $expectedPath) { | ||
| $route = $this->getDm()->find(null, $expectedPath); | ||
|
|
||
| $this->assertNotNull($route, 'Found route with path ' . $expectedPath); | ||
| $this->assertInstanceOf('Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRoute', $route); | ||
|
|
||
| $content = $route->getContent(); | ||
|
|
||
| $this->assertNotNull($content); | ||
| $this->assertInstanceOf('Symfony\Cmf\Bundle\RoutingAutoBundle\Tests\Resources\Document\Article', $content); | ||
|
|
||
| // We havn't loaded the translation for the document, so it is always in the default language | ||
| $this->assertEquals('Hello everybody!', $content->title); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony CMF package. | ||
| * | ||
| * (c) 2011-2013 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\RoutingAutoBundle\Tests\Resources\Document; | ||
|
|
||
| use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCR; | ||
| use Doctrine\Common\Collections\ArrayCollection; | ||
|
|
||
| /** | ||
| * @PHPCR\Document(translator="child", referenceable=true) | ||
| */ | ||
| class Category | ||
| { | ||
| /** | ||
| * @PHPCR\Id() | ||
| */ | ||
| public $path; | ||
|
|
||
| /** | ||
| * @PHPCR\Referrers( | ||
| * referringDocument="Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Phpcr\Route", | ||
| * referencedBy="content" | ||
| * ) | ||
| */ | ||
| public $routes; | ||
|
|
||
| /** | ||
| * @PHPCR\String(translated=true) | ||
| */ | ||
| public $title; | ||
|
|
||
| /** | ||
| * @PHPCR\Locale() | ||
| */ | ||
| public $locale; | ||
|
|
||
| /** | ||
| * @PHPCR\ReferenceMany(targetDocument="Symfony\Cmf\Bundle\RoutingAutoBundle\Tests\Resources\Document\Article") | ||
| */ | ||
| public $articles = array(); | ||
|
|
||
| public function getTitle() | ||
| { | ||
| return $this->title; | ||
| } | ||
| } |
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.
missing full stop