port features from simple cms into routing bundle to simplify things

[dbu]

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]

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.

[dbu]

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]

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

[dbu]

right. created #211

[wouterj]

Will this be merged into 1.2?

[dbu]

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]

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]

@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?).

[wouterj]

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 :)

[lsmith77]

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

[dbu]

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

[dbu]

@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?

[wouterj]

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()

[wouterj]

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

[dbu]

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

[wouterj]

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

[dbu]

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]

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

[dbu]

@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]

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

[dbu]

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]

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

[lsmith77]

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

[lsmith77]

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

[lsmith77]

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]

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]

fixed

[dbu]

added limit configuration for the candidates.