symfony routing #800
symfony routing #800
Conversation
Note: for now, we just set the application name from this "controller". We allready have our module, action and language variables here. These should be passed to the Action class.
The router is the only place in Fork where we define applications. There's no need for unnecessary defensive programming.
They still give problems at this moment with the Symfony routing component.
This way, we'll be able to use the Symfony routing from the Frameworkbundle.
Only the request handling and the defineForkConstants are still custom functions.
This way, it'll be easier to check if all needed services are registered correctly.
|
This pull requests also contains integration with the Symfony console. I closed my other pull request integrating it (#733) in favor of this one. These commands are allready available:
Not all commands work as intended yet. This was added in the first place for the container:debug command. This command allready works perfectly. |
This way, we're sure all our functions mimic the Symfony kernel and we only need to overwrite functions that have a different implementation.
| path: /api | ||
| defaults: | ||
| _controller: ApplicationRouting::apiController | ||
| frontend: |
There was a problem hiding this comment.
you may also want to look at Symfony2 CMF Bundle in another step. This would enable you to no longer need a catch all route like this but instead do lookups if a page exists in the routing layer itself. This would also make it possible to eventually support different controllers for different types of content.
http://symfony.com/doc/master/cmf/components/routing/index.html
There was a problem hiding this comment.
Thanks for the tip. I was allready looking at the chainRouter for further work, but the CMF bundle could indeed be a good step in the direction we want to take.
We stille have a lot of work though, before we can fully switch our routing to the Symfony stack. This is just a small step in the right direction.
There was a problem hiding this comment.
yeah, would take it step by step. ie. this PR already touches a lot of stuff.
|
With |
|
In an old version we had our index page out of the document root. We changed this to help people use ForkCMS on cheap shared hosting where you can only upload files in your document root. I am an advocate for moving the index page in a web folder, but I guess this is something we'll have to discuss with the other ForkCMS contributors/owners. Poke @forkcms/core-contributors |
|
I think its ok to have it as the default in the root but make it possible to have it outside the root. Having it outside of the root improves security considerably, something that novice users especially will appreciate. |
|
:+1000000000000000: for moving the index.php into the web-folder |
|
Note to self: The ajax and cronjob endpoints have been changed in the core, I have to change it too in this PR. |
|
👍 |
|
@tijsverkoyen does your 👍 mean we can merge this tomorrow before releasing the 3.7.2 version? |
|
yep, but release it as 3.8 On 30 Jul 2014, at 14:59, Wouter Sioen notifications@github.com wrote:
|
|
@tijsverkoyen Why would you release it as 3.8? There aren't any backwards compatibility breaks and 95% of the Fork users will never notice the difference. |
|
Well I thought this was a major change, but if it is fully backward compatible you can release it as 3.7.2 On 30 Jul 2014, at 15:19, Wouter Sioen notifications@github.com wrote:
|
|
There were still some issues with the installer, but these are now fixed. I'll still test the other pull requests assigned to me and then, we'll start releasing the new version. |
Integrating the Symfony routing component. This will make it easier to route some code to full stack Symfony (Bundles) when other code is still run on the legace code of Fork.
Things that should be done in a follow up request
Note: since frontend url's are that custom, only the backend url's are generated with the Symfony routing component (for now).
Related issue: #409
Not all tasks in this commit are done, but I think this already changes enough to not work futher on this pull request, and do further integration in other branches.