Sandbox::https://github.com/contao/core-bundle/pull/18

tristanlins · 2014-11-02T16:42:02Z

And finally here we goes! This PR implements backend and frontend routing, because both depend on each other.

The backend routing was original loaded through the app/config/routing.yml. Now it is autoloaded by the new BundlesLoader, the usage is documented here.

The frontend routing is done through the CMF router.

I add a custom ChainedRouteProvider to give the ability of providing multiple providers at once. The usage is documented here. The chain provider is only used if its needed, see AddRouteProvidersPass:53-56.

The PageRouteProvider generate the routes for the page structure. The real generation of the routes is decoupled through events. The CreatePageRoutesSubscriber create the routes for the default page types. Extension developers can use the event to create their own routes for the custom types.

Open questions and TODOs

Support fallback to regular page routing for custom types?!
Support custom page types in the FrontendController?!
Re-implement the page types logic (e.g. from PageRegular) in the FrontendController?!
Redirect index page to / url?!
Thanks to the redirecting backend entry points, e.g. contao/main.php, the referrer logic and so the "back"-link is broken :-\
The config/routing.yml is located in the contao resources path. Is this a good idea?
The page visibility is not respected yet!

I wrote a documentation draft, you can see it here: contao/docs#176

Related to contao/contao#32

Setting the $GLOBALS['objPage'] in the listener helps improve the legacy compatibility for custom routes.

Summarize arrays [..] + [..] preserve the values from the first array that define a key. ['key' => A] + ['key' => 'B'] result in ['key' => A], contrary array_merge() would result in ['key' => 'B'].

…page.

tristanlins · 2014-11-02T19:56:01Z

I implemented support for custom page types, see 92106ff
And the /index.html page will now be converted into a forward page to /, see 82f1f54

aschempp · 2014-11-02T20:36:56Z

I think this explanation is a copy-leftover?

Youre absolutely right ;-D

aschempp · 2014-11-02T20:40:30Z

I haven't fully understood everything yet, maybe you can explain the following questions:

auto_item was only added in Contao 3 if the number of parameters was even. I think the current implementation will always use auto_item, no matter the number of parameters?
Why is the routing.yml file located in the Contao resources and not the bundle config folder?
I can't find the RedirectController file, but it seems used multiple times?
Why do you use underscore prefixes for route parameters?

Also:

I find the name CoreModuleEvents very confusing. I had to read multiple times to realize this is not related to the events module. Maybe it should be CoreBundleEvents?

tristanlins · 2014-11-02T21:50:12Z

auto_item was only added in Contao 3 if the number of parameters was even. I think the current implementation will always use auto_item, no matter the number of parameters?

You're wrong. I match this behavior through the regexp, see the _auto_item and _path_parameters patterns at CreatePageRoutesSubscriber.php#L375-L378

_path_parameters is defined as (/{key}/{value})* while _auto_item is just (/{value})?.
In other words the regexp will match this way:

URL	_auto_item	_path_parameters
`/alias.html`	~	~
`/alias/foo.html`	`/foo`	~
`/alias/foo/bar.html`	~	`/foo/bar`
`/alias/foo/bar/zap.html`	`/foo`	`/bar/zap`
`/alias/bar/zap/king/kong.html`	~	`/bar/zap/king/kong`
`/alias/foo/bar/zap/king/kong.html`	`/foo`	`/bar/zap/king/kong`

Simply regexp magic :-)

tristanlins · 2014-11-02T21:53:24Z

Why is the routing.yml file located in the Contao resources and not the bundle config folder?

Because I use the ContaoBundle::getContaoResourcesPath() to determine the parent path. I myself am not happy with this. See my comment at BundlesLoader.php#L72.

tristanlins · 2014-11-02T21:54:20Z

I can't find the RedirectController file, but it seems used multiple times?

Because it is part of the CMF Bundle ;-)
https://github.com/symfony-cmf/RoutingBundle/blob/master/Controller/RedirectController.php

tristanlins · 2014-11-02T21:56:31Z

Why do you use underscore prefixes for route parameters?

Because these parameters are for internal use only, not for direct use by other bundle developers. Similar to the _locale and _format parameters.

tristanlins · 2014-11-02T21:59:13Z

I find the name CoreModuleEvents very confusing. I had to read multiple times to realize this is not related to the events module. Maybe it should be CoreBundleEvents?

Mh, its a valid point. Once we rename module-core back to core-bundle the name CoreBundleEvents is right.

aschempp · 2014-11-03T08:17:08Z

Even though this works, a string comparison would be way more obvious what it does.

return '' !== $this->getFormatPattern();

Personally I'm absolutely fine with a bool cast. Let's see what the others say.

Same for me, I like the bool cast more as it IMO describes best that we are returning bool here without to have to read all the code.

I'm with @aschempp on this one.

aschempp · 2014-11-03T08:23:37Z

I can't find the RedirectController file, but it seems used multiple times?

Because it is part of the CMF Bundle ;-)
https://github.com/symfony-cmf/RoutingBundle/blob/master/Controller/RedirectController.php

Ok, but our PageModel does not (yet) implement the RedirectRouteInterface?

leofeyer · 2014-11-04T20:25:25Z

Wow, this looks great. Thank you very much!

It seems to rely on Template::getResponse() though; a feature which I have removed after I have asked you guys on Twitter :) Do I need to re-implement it? I figured that @aschempp's approach did not need the getResponse() method?

tristanlins · 2014-11-05T06:34:46Z

@leofeyer you asked for the page types Page::getResponse, not Template::getResponse???
https://twitter.com/leofeyer/status/528830156958625792

This is still an open todo:

Re-implement the page types logic (e.g. from PageRegular) in the FrontendController?!

aschempp · 2014-11-05T09:25:39Z

@tristanlins check out how I added routing by type and the fallback:
https://github.com/contao/contao/pull/24/files#diff-bf0e70bcef1a5d5b2f87289220a51108R17

leofeyer · 2014-11-07T14:11:36Z

I have merged the changes into the feature/routing branch as discussed on Mumble. I will now re-add the Template::getResponse() methods.

leofeyer · 2014-11-07T14:47:47Z

Apparently, you are relying on the ContaoLoader in your PR?

Fatal error: Class 'Contao\CoreBundle\Config\Loader\ContaoLoader' not found

I figured that your PR renders the Contao loader redundant, therefore I have not merged it when I was restructuring the repos. Do I need to restore it?

tristanlins · 2014-11-11T19:53:46Z

The class ContaoLoader should not be necessary. I did some work on the feature/routing branch, but it's not complete, I was a little busy the last days, but I will complete my work next week.
Anyway we should close this PR in favor of the feature/routing branch and the new PRs, e.g. #20

aschempp · 2014-12-13T10:07:40Z

@leofeyer can you please rebase the feature/routing branch on top of latest develop?

tristanlins · 2014-12-14T15:43:08Z

I will do, please stand by...

tristanlins · 2014-12-14T15:58:02Z

My PR is now up to date and rebased against current develop.

Contrary to the current feature/routing branch, my branch contains:

Fix little bugs.
Enable rewriteURL by default.
A generic route resource loader, see class Routing\ResourceLoader
The ability to load custom route loader services by tag name contao.routing.loader, see example in services.yml

Toflar · 2014-12-15T07:23:29Z

Wollten wir nicht eine generelle Möglichkeit bieten, das System zu beenden mit einer ResponseException weil das Entwickler auch brauchen in ihren eigenen Modulen? Dann könnte man die hier wiederverwenden? :)

Denke ich auch, aber das sollten wir separat behandeln.

aschempp · 2014-12-15T10:23:01Z

I have reviewed and hopefully understood the routing now while working on custom page controllers. I have also found again my original drafts:
aschempp/contao@bc800f9
aschempp/contao-bundle@b2f400c

They are pretty similar. What makes your implementation complex is the contao.routing.loader because we discussed in our call not to automatically load the Resources/config/routing.yml file of a contao extension.
I think we should discuss that again. Your implementation obviously will work, but I think it's too complex for just loading a custom routing file. I know you implemented exactly what we talke about ;-)

What do you think about specifying the bundle's routing.yml file in the autoload.json instead? I think that could solve all problems (hard-coded path, unnecessary complex loading).

tristanlins · 2014-12-15T11:59:03Z

I don't think it is complex for third party developers, because they just need to define a service and define their own routing file as second parameter. This is very similar to "How to Create a custom Route Loader", expect that you do not need to implement the loader ;-)

tristanlins · 2014-12-15T12:15:26Z

While reading the documentation again, I found an optimisation to the resource loader, it is not necessary anymore to define the loader service as first parameter :-)

aschempp · 2014-12-15T12:16:36Z

I didn't mean to say its too complex for the developer, but the complexity does not add any functionality compared to automatically load a routing.yaml file if we know the path.

tristanlins · 2014-12-15T12:18:06Z

...but the complexity does not add any functionality compared to automatically load a routing.yaml file if we know the path.

You can generate routes with a custom loader without any routing.yml.

aschempp · 2014-12-15T12:21:02Z

You can generate routes with a custom loader without any routing.yml.

I know, but that's rather unlikely (they're static after all) and then our ResourceLoader would be useless. Just saying we should discuss that again.

leofeyer · 2015-03-02T19:56:22Z

@aschempp What about this ticket? Can I close it?

aschempp · 2015-03-02T19:58:02Z

I think so. I still got the feature branches in my fork to continue on the dynamic router.

@Toflar

Description ----------- With @Toflar we have discovered by accident that if a URL is double-encoded (for some reason, doesn't matter) the Contao's `RouteProvider` will eventually throw an error trying to query a database. ``` URL original: drachenlochmuseum-v%25c3%25a4ttis.html URL decoded: drachenlochmuseum-v%c3%a4ttis.html URL decoded 2nd time: drachenlochmuseum-vättis.html ``` The decoded URL is used in the database query and that fails because the database driver would like to replace wildcards `%c` with parameters that were not provided. Stack trace: ``` Exception: Too few arguments to build the query string #27 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Database/Statement.php(304): replaceWildcards #26 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Database/Statement.php(249): execute #25 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Model.php(1102): find #24 vendor/contao/core-bundle/src/Resources/contao/library/Contao/Model.php(973): findBy #23 vendor/contao/core-bundle/src/Framework/Adapter.php(38): __call #22 vendor/contao/core-bundle/src/Routing/RouteProvider.php(493): findPages #21 vendor/contao/core-bundle/src/Routing/RouteProvider.php(88): getRouteCollectionForRequest #20 vendor/contao/core-bundle/src/Routing/LegacyRouteProvider.php(43): getRouteCollectionForRequest #19 vendor/symfony-cmf/routing/src/NestedMatcher/NestedMatcher.php(141): matchRequest #18 vendor/contao/core-bundle/src/Routing/Matcher/LegacyMatcher.php(69): matchRequest #17 vendor/symfony-cmf/routing/src/DynamicRouter.php(271): matchRequest #16 vendor/symfony-cmf/routing/src/ChainRouter.php(188): doMatch #15 vendor/symfony-cmf/routing/src/ChainRouter.php(158): matchRequest #14 vendor/symfony/http-kernel/EventListener/RouterListener.php(115): onKernelRequest #13 vendor/symfony/event-dispatcher/EventDispatcher.php(212): doDispatch #12 vendor/symfony/event-dispatcher/EventDispatcher.php(44): dispatch #11 vendor/symfony/http-kernel/HttpKernel.php(126): handleRaw #10 vendor/symfony/http-kernel/HttpKernel.php(67): handle #9 vendor/symfony/http-kernel/Kernel.php(198): handle #8 vendor/symfony/http-kernel/HttpCache/SubRequestHandler.php(85): handle #7 vendor/symfony/http-kernel/HttpCache/HttpCache.php(448): forward #6 vendor/symfony/framework-bundle/HttpCache/HttpCache.php(57): forward #5 vendor/symfony/http-kernel/HttpCache/HttpCache.php(420): fetch #4 vendor/contao/manager-bundle/src/HttpKernel/ContaoCache.php(46): fetch #3 vendor/symfony/http-kernel/HttpCache/HttpCache.php(317): lookup #2 vendor/symfony/http-kernel/HttpCache/HttpCache.php(192): handle #1 vendor/friendsofsymfony/http-cache/src/SymfonyCache/EventDispatchingHttpCache.php(98): handle #0 web/app.php(58): null ``` Commits ------- 8ae25820 Fix a potential error if the URL has percentage in it 8caaf252 Fix unit tests 509f7624 Correctly encode the page aliases

… #7248) Description ----------- If a `tl_article` record still contains `keywords` from previous Contao versions and you reference this article in an `article` content element, the following warning will appear in the back end: ``` ErrorException: Warning: Undefined global variable $TL_KEYWORDS #24 /vendor/contao/core-bundle/src/Resources/contao/modules/ModuleArticle.php(207): Contao\ModuleArticle::compile #23 /vendor/contao/core-bundle/src/Resources/contao/modules/Module.php(214): Contao\Module::generate #22 /vendor/contao/core-bundle/src/Resources/contao/modules/ModuleArticle.php(70): Contao\ModuleArticle::generate #21 /vendor/contao/core-bundle/src/Resources/contao/library/Contao/Controller.php(550): Contao\Controller::getArticle #20 /vendor/contao/core-bundle/src/Resources/contao/elements/ContentArticle.php(30): Contao\ContentArticle::generate #19 /vendor/contao/core-bundle/src/Resources/contao/library/Contao/Controller.php(622): Contao\Controller::getContentElement #18 /var/cache/prod/contao/dca/tl_content.php(318): tl_content::addCteType #17 /vendor/contao/core-bundle/src/Resources/contao/drivers/DC_Table.php(4717): Contao\DC_Table::parentView #16 /vendor/contao/core-bundle/src/Resources/contao/drivers/DC_Table.php(313): Contao\DC_Table::showAll #15 /vendor/contao/core-bundle/src/Resources/contao/classes/Backend.php(667): Contao\Backend::getBackendModule #14 /vendor/contao/core-bundle/src/Resources/contao/controllers/BackendMain.php(168): Contao\BackendMain::run #13 /vendor/contao/core-bundle/src/Controller/BackendController.php(49): Contao\CoreBundle\Controller\BackendController::mainAction #12 /vendor/symfony/http-kernel/HttpKernel.php(163): Symfony\Component\HttpKernel\HttpKernel::handleRaw #11 /vendor/symfony/http-kernel/HttpKernel.php(75): Symfony\Component\HttpKernel\HttpKernel::handle #10 /vendor/symfony/http-kernel/Kernel.php(202): Symfony\Component\HttpKernel\Kernel::handle #9 /vendor/symfony/http-kernel/HttpCache/SubRequestHandler.php(86): Symfony\Component\HttpKernel\HttpCache\SubRequestHandler::handle #8 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(481): Symfony\Component\HttpKernel\HttpCache\HttpCache::forward #7 /vendor/symfony/framework-bundle/HttpCache/HttpCache.php(73): Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache::forward #6 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(454): Symfony\Component\HttpKernel\HttpCache\HttpCache::fetch #5 /vendor/contao/manager-bundle/src/HttpKernel/ContaoCache.php(66): Contao\ManagerBundle\HttpKernel\ContaoCache::fetch #4 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(348): Symfony\Component\HttpKernel\HttpCache\HttpCache::lookup #3 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(226): Symfony\Component\HttpKernel\HttpCache\HttpCache::handle #2 /vendor/friendsofsymfony/http-cache/src/SymfonyCache/EventDispatchingHttpCache.php(96): Contao\ManagerBundle\HttpKernel\ContaoCache::handle #1 /web/index.php(44): require #0 /web/app.php(13): null ``` This is because `$GLOBALS['TL_KEYWORDS']` is only initialised in the front end within `PageRegular::prepare()`. This PR simply ignores it if not set. Commits ------- e54dc2b8 fix warning when legacy keywords are present

…#7268) Description ----------- Got this erro in my logs: ``` RuntimeException: The Symfony container is not available, did you initialize the Contao framework? in vendor/contao/core-bundle/contao/library/Contao/System.php:143 Stack trace: #0 vendor/contao/core-bundle/contao/library/Contao/System.php(97): Contao\System->import('Contao\\Config', 'Config') #1 vendor/contao/core-bundle/contao/library/Contao/DcaExtractor.php(105): Contao\System->__construct() #2 vendor/contao/core-bundle/contao/library/Contao/DcaExtractor.php(140): Contao\DcaExtractor->__construct('tl_opt_in') #3 vendor/contao/core-bundle/contao/library/Contao/Model.php(291): Contao\DcaExtractor::getInstance('tl_opt_in') #4 vendor/contao/core-bundle/contao/library/Contao/Model.php(1084): Contao\Model::getUniqueFields() #5 vendor/contao/core-bundle/contao/models/OptInModel.php(87): Contao\Model::findBy(Array, 1716888783, Array) #6 vendor/contao/core-bundle/src/Framework/Adapter.php(38): Contao\OptInModel::findExpiredTokens() #7 vendor/contao/core-bundle/src/OptIn/OptIn.php(76): Contao\CoreBundle\Framework\Adapter->__call('findExpiredToke...', Array) #8 vendor/contao/core-bundle/src/Cron/PurgeOptInTokensCron.php(30): Contao\CoreBundle\OptIn\OptIn->purgeTokens() #9 vendor/contao/core-bundle/src/Cron/CronJob.php(44): Contao\CoreBundle\Cron\PurgeOptInTokensCron->__invoke('cli') #10 vendor/contao/core-bundle/src/Cron/Cron.php(197): Contao\CoreBundle\Cron\CronJob->__invoke('cli') #11 vendor/contao/core-bundle/src/Cron/Cron.php(182): Contao\CoreBundle\Cron\Cron->executeCrons(Array, 'cli', Object(Closure)) #12 vendor/contao/core-bundle/src/Cron/Cron.php(97): Contao\CoreBundle\Cron\Cron->doRun(Array, 'cli', false) #13 vendor/contao/core-bundle/src/Command/CronCommand.php(53): Contao\CoreBundle\Cron\Cron->run('cli', false) #14 vendor/symfony/console/Command/Command.php(326): Contao\CoreBundle\Command\CronCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput)) #15 vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput)) #16 vendor/symfony/framework-bundle/Console/Application.php(126): Symfony\Component\Console\Application->doRunCommand(Object(Contao\CoreBundle\Command\CronCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput)) #17 vendor/symfony/console/Application.php(324): Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand(Object(Contao\CoreBundle\Command\CronCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput)) #18 vendor/symfony/framework-bundle/Console/Application.php(80): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput)) #19 vendor/symfony/console/Application.php(175): Symfony\Bundle\FrameworkBundle\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput)) #20 vendor/contao/manager-bundle/bin/contao-console(40): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput)) #21 vendor/bin/contao-console(119): #22 ``` Commits ------- 9ee76a07 Initialize the Contao framework when working with opt-in tokens

tristanlins added 15 commits November 1, 2014 21:13

Make the backend accessible through symfony controllers.

0cc96b3

Make the contao kernel terminable.

3420c10

Use dynamic bundles route loader.

db84811

Use router to generate login url when backend user auth failed.

fe13841

Use router to generate backend menu item urls.

90f3731

Fix share directory is located in web/ sub directory.

9c582cb

Add backend routing definition.

23a49c1

Set the $GLOBALS['objPage'] in the parameter filter listener.

73bf275

Setting the $GLOBALS['objPage'] in the listener helps improve the legacy compatibility for custom routes.

Make the bundles routing loader one time loadable.

4a17c2d

Little code reformat.

360cfea

Add a prepends loader for easy prepend extension configs.

ed1c49b

Integrate the CMF routing.

6b24838

Support custom page type in the new frontend controller.

92106ff

Fix array addition order.

0e60710

Summarize arrays [..] + [..] preserve the values from the first array that define a key. ['key' => A] + ['key' => 'B'] result in ['key' => A], contrary array_merge() would result in ['key' => 'B'].

Convert page with alias 'index' into forward page to the parent root …

82f1f54

…page.

Respect setting of the redirect type, temporary or permanent.

8ca0052

aschempp reviewed Nov 2, 2014
View reviewed changes

Remove leading "/" from _auto_item route attribute.

d1c14c7

tristanlins added 3 commits November 2, 2014 23:00

Fix $this->$this->... ;-D

1cc19a9

Rename class CoreModuleEvents to CoreBundleEvents.

e6451ff

Update description for CoreBundleEvents::CREATE_PAGE_ROUTES.

bf04d39

aschempp reviewed Nov 3, 2014
View reviewed changes

leofeyer added the feature label Nov 4, 2014

leofeyer added this to the 4.0.0 milestone Nov 4, 2014

tristanlins added 2 commits November 6, 2014 22:41

Fix BundlesLoader::supports() check.

d4c3db1

Support the new loader type.

3286c51

Toflar reviewed Dec 15, 2014
View reviewed changes

leofeyer closed this Mar 2, 2015

This was referenced Mar 23, 2015

Draft of a backend routing handler #92

Closed

Use the Symfony request response #93

Closed

Added CmfRoutingBundle for dynamic routing #102

Closed

Implement backend and frontend routing #110

Closed

Conversation

tristanlins commented Nov 2, 2014

Uh oh!

tristanlins commented Nov 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschempp commented Nov 2, 2014

Uh oh!

tristanlins commented Nov 2, 2014

Uh oh!

tristanlins commented Nov 2, 2014

Uh oh!

tristanlins commented Nov 2, 2014

Uh oh!

tristanlins commented Nov 2, 2014

Uh oh!

tristanlins commented Nov 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschempp commented Nov 3, 2014

Uh oh!

leofeyer commented Nov 4, 2014

Uh oh!

tristanlins commented Nov 5, 2014

Uh oh!

aschempp commented Nov 5, 2014

Uh oh!

leofeyer commented Nov 7, 2014

Uh oh!

leofeyer commented Nov 7, 2014

Uh oh!

tristanlins commented Nov 11, 2014

Uh oh!

aschempp commented Dec 13, 2014

Uh oh!

tristanlins commented Dec 14, 2014

Uh oh!

tristanlins commented Dec 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschempp commented Dec 15, 2014

Uh oh!

tristanlins commented Dec 15, 2014

Uh oh!

tristanlins commented Dec 15, 2014

Uh oh!

aschempp commented Dec 15, 2014

Uh oh!

tristanlins commented Dec 15, 2014

Uh oh!

aschempp commented Dec 15, 2014

Uh oh!

leofeyer commented Mar 2, 2015

Uh oh!

aschempp commented Mar 2, 2015

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants