port features from simple cms into routing bundle to simplify things #210

dbu · 2014-01-26T20:51:47Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes (small ones)
Deprecations?	yes
Tests pass?	not yet
Fixed tickets	#156, #149, symfony-cmf/symfony-cmf#182
License	MIT
Doc PR	symfony-cmf/symfony-cmf-docs#430

This attempts to port some features from SimpleCmsBundle to the core route, to simplify things and because they make sense outside of the simplecms context too.

The most important change is that the RouteProvider and the PHCPR-ODM listeners now support more than one prefix. This should hopefully get us rid of the whole duplicated router business with all the problems it brought.

I also propose to add an option to optionally have the locale listener automatically set the addLocalePattern on routes that have no locale from their id - it is a rather extreme case that you would need to set that individually per route.

Update configuration for multiple base paths
Check the locales support
Get tests to succeed again
Check for leftovers in the PR
Adjust admin class
Add BC code for the move to $options and document migration in UPGRADE.md (should we keep even a BC mapping, or only the methods?)
Refactor ORM to also use candidates

/cc @benglass i would love to hear your feedback on this.

dbu · 2014-01-26T20:55:52Z

Doctrine/Phpcr/LocaleRouteProvider.php

+ *
+ * @author Lukas Kahwe Smith <[email protected]>
+ */
+class LocaleRouteProvider extends RouteProvider


i moved this over from SimpleCmsBundle. not sure if we should integrate it into the RouteProvider - felt like a bit too much. we could have an option to make the RoutingBundle use this one instead of the simple one.

with #213, most of this could go into the Candidates strategy. not all though, the configureLocale thing would need to stay - or we need some postLoad method on Candidates

dantleech · 2014-01-26T21:13:44Z

Doctrine/Phpcr/IdPrefixListener.php

+ *
+ * This listener knows about the RouteProvider and uses its prefixes to
+ * identify routes that could need the prefix. In case prefixes overlap, the
+ * order matters as the first matching prefix is taken.
 *
 * @author David Buchmann <[email protected]>
 */
 class IdPrefixListener


Not related to this PR, but a Subscriber might make more sense here as this class listens to multiple events.

right. created #211

wouterj · 2014-03-02T13:32:21Z

Will this be merged into 1.2?

dbu · 2014-03-02T13:37:42Z

i really hope to do it, yes. it is rather disruptive for the simple cms configuration, but we really got it wrong in the previous version.

i am focussing on releasing the phpcr toolchain. this PR is the next thing once thats done.

dbu · 2014-03-24T22:24:44Z

squashed the commits into meaningful chunks, and integrated the candidates strategy part. the candidate code itself is going into Routing as its not symfony specific.

still need to sort out how to handle the locales together with the prefixes, and use the candidates pattern to move some code out of the orm provider too.

dbu · 2014-03-25T21:02:05Z

Doctrine/Orm/RouteProvider.php

+        $routes = $this->getRouteRepository()->findByStaticPrefix($candidates, array('position' => 'ASC'));
+        /** @var $route Route */
+        foreach ($routes as $route) {
+            // TODO: is this still relevant? why does phpcr-odm not do something similar?


@wouterj you helped doing this. would you have an idea why we did this? @lsmith77 or do you remember something? seems we don't do it in phpcr (anymore?).

it parses the url to get the extension (if available). It assumes the extension refers to the format and then sets that format as a route parameter of the route.

I don't like that method. It is heavily based on conventions. And if someone would need to do this, it should make a placeholder in the variable part of the route for the _format.

Btw, I only helped improving test coverage of the ORM provider, I didn't code it afaik :)

i am pretty sure we had something similar in phpcr odm providers as well. and yes its naiv and yes .. maybe we should instead encourage the use of .{_format} instead

i found the changelog entry for phpcr - will remove it for the orm
provider and add a changelog note.

dbu · 2014-03-29T13:49:05Z

DependencyInjection/Configuration.php

+                                            $v['route_basepaths'] = array_unique($base);
+                                            unset($v['route_basepath']);
+
+                                            return $v;


@wouterj there is no easier way to do this, right? i kind of collide with the fixXmlConfig thing. should i even remove fixXmlConfig('route_basepath') above, as it won't ever happen?

I would handle it something different. Or you configure the route_basepaths option or the route_basepath, but not both. It'll look like:

->validate() ->ifTrue(function ($v) { isset($v['route_basepath']) && isset($v['route_basepaths']); }) ->thenInvalid('Found values for both "route_basepath" and "route_basepaths", use "route_basepaths" instead.') ->end()

And then you can remove the normalizer and just use the fixXmlConfig

oh, that will "accidentally" fix it even if i am using the old format with yml / php configuration, right?

It will fix it if you use the old format, and it'll break if you use both the old and new format

dbu · 2014-03-29T15:06:43Z

this seems to near completion. any last feedback? any input on the orm test failure?

i will create PR on simplecms and corebundle and sandbox to adjust to this.

lsmith77 · 2014-03-30T11:43:02Z

there seems to be a problem in the test setup for the ORM tests

dbu · 2014-03-31T09:08:42Z

@lsmith77 yes, the orm test is weird. when i only run the orm tests, it works fine. but after the phpcr tests have run, it fails. i have no clue what is happening.

lsmith77 · 2014-03-31T09:11:05Z

heh .. odd but so you can reproduce the issue locally?

dbu · 2014-03-31T09:13:13Z

yes. not sure if i should be happy about that or not. i have no clue what is happening - seems like a very basic autoloading f***up or reflection going crazy. or the phpunit mocking thing. or the testing component getting messed up by something. if you have any ideas, i would be happy, i have no clue.

lsmith77 · 2014-03-31T12:07:49Z

FYI https://twitter.com/lsmith/status/450561719791026176

lsmith77 · 2014-03-31T18:54:55Z

will check the test issue out now to try and fit it.

lsmith77 · 2014-03-31T19:13:40Z

seem to be related to some magic inside the testing component. investigating further /cc @wouterj @dantleech

lsmith77 · 2014-03-31T19:22:10Z

if I disable the exception that is thrown in the Configuration class, I end up with the following:

Fatal error: Call to undefined method Doctrine\Orm\EntityRepository::findAll() in /Users/lsmith/htdocs/cmf-sandbox/vendor/symfony-cmf/routing-bundle/Symfony/Cmf/Bundle/RoutingBundle/Tests/Functional/Doctrine/Orm/OrmTestCase.php on line 37

notice Doctrine\Orm\EntityRepository instead of Doctrine\ORM\EntityRepository ..

lsmith77 · 2014-03-31T19:23:39Z

diff --git a/Tests/Unit/Doctrine/Orm/RouteProviderTest.php b/Tests/Unit/Doctrine/Orm/RouteProviderTest.php
index 56f7ec6..0728f3e 100644
--- a/Tests/Unit/Doctrine/Orm/RouteProviderTest.php
+++ b/Tests/Unit/Doctrine/Orm/RouteProviderTest.php
@@ -59,7 +59,7 @@ class RouteProviderTest extends CmfUnitTestCase
         $this->route2Mock = $this->buildMock('Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Orm\Route');
         $this->objectManagerMock = $this->getMock('Doctrine\Common\Persistence\ObjectManager');
         $this->managerRegistryMock = $this->getMock('Doctrine\Common\Persistence\ManagerRegistry');
-        $this->objectRepositoryMock = $this->getMock('Doctrine\Orm\EntityRepository', array('findByStaticPrefix', 'findOneBy', 'findBy'));
+        $this->objectRepositoryMock = $this->getMock('Doctrine\ORM\EntityRepository', array('findByStaticPrefix', 'findOneBy', 'findBy'));
         $this->candidatesMock = $this->getMock('Symfony\Cmf\Component\Routing\Candidates\CandidatesInterface');
         $this->candidatesMock
             ->expects($this->any())

now I am getting some other failures .. investigating

lsmith77 · 2014-03-31T19:26:17Z

fixed

dbu · 2014-03-31T21:03:00Z

added limit configuration for the candidates.

port features from simple cms into routing bundle to simplify things

dbu reviewed Jan 26, 2014
View reviewed changes

dbu mentioned this pull request Jan 26, 2014

Ability to have multiple basepaths symfony-cmf/symfony-cmf#182

Closed

dantleech reviewed Jan 26, 2014
View reviewed changes

dbu mentioned this pull request Feb 1, 2014

[WIP] candidates strategy #213

Closed

This was referenced Feb 8, 2014

Enable the ContentObject provider to work with translated referrers symfony-cmf/routing-auto-bundle#63

Closed

refactor common features into RoutingBundle symfony-cmf/simple-cms-bundle#98

Merged

This was referenced Mar 2, 2014

[CMF] Start new SE symfony/symfony-standard#633

Closed

[WIP] Start new SE symfony-cmf/standard-edition#1

Merged

This was referenced Mar 23, 2014

Seperating repositories (content, routes etc.) for different websites (1 database) doctrine/DoctrinePHPCRBundle#131

Closed

adding candidates subsystem symfony-cmf/Routing#98

Merged

dbu added 2 commits March 25, 2014 21:59

port features from simple cms into routing bundle to simplify things

d81035a

convert settings to options

3eef9cd

dbu reviewed Mar 25, 2014
View reviewed changes

dbu reviewed Mar 29, 2014
View reviewed changes

dbu changed the title ~~[WIP] port features from simple cms into routing bundle to simplify things~~ port features from simple cms into routing bundle to simplify things Mar 29, 2014

add changelog and upgrade instructions

623a6ad

dbu mentioned this pull request Mar 31, 2014

adjust to routing cleanup symfony-cmf/cmf-sandbox#244

Merged

lsmith77 mentioned this pull request Mar 31, 2014

[WCM] Reflect SeoBundle configuration changes symfony-cmf/core-bundle#133

Merged

fix ORM tests

badf1d8

dbu added 4 commits March 31, 2014 22:10

cleanup sonata admin

f3c27b8

fix route filter name. via @psytraxx

0b2ce04

adjust to feedback for configuration

b6729db

support candidates limit

a1205ec

dbu and others added 3 commits March 31, 2014 23:04

cleaning up redirect route admin a bit

dbbd077

add option for locale pattern

6e546e1

routing component candidates branch has been merged

2c9293e

lsmith77 added a commit that referenced this pull request Apr 1, 2014

Merge pull request #210 from symfony-cmf/cleanup-routing

98a70fe

port features from simple cms into routing bundle to simplify things

lsmith77 merged commit 98a70fe into master Apr 1, 2014

lsmith77 deleted the cleanup-routing branch April 1, 2014 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port features from simple cms into routing bundle to simplify things #210

port features from simple cms into routing bundle to simplify things #210

dbu commented Jan 26, 2014

dbu Jan 26, 2014

dbu Feb 1, 2014

dantleech Jan 26, 2014

dbu Jan 30, 2014

wouterj commented Mar 2, 2014

dbu commented Mar 2, 2014

dbu commented Mar 24, 2014

dbu Mar 25, 2014

wouterj Mar 25, 2014

lsmith77 Mar 26, 2014

dbu Mar 26, 2014

dbu Mar 29, 2014

wouterj Mar 29, 2014

wouterj Mar 29, 2014

dbu Mar 29, 2014

wouterj Mar 29, 2014

dbu commented Mar 29, 2014

lsmith77 commented Mar 30, 2014

dbu commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

dbu commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

dbu commented Mar 31, 2014

port features from simple cms into routing bundle to simplify things #210

port features from simple cms into routing bundle to simplify things #210

Conversation

dbu commented Jan 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Mar 2, 2014

dbu commented Mar 2, 2014

dbu commented Mar 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Mar 29, 2014

lsmith77 commented Mar 30, 2014

dbu commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

dbu commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

lsmith77 commented Mar 31, 2014

dbu commented Mar 31, 2014