Skip to content

[DX][FrameworkBundle] Simpler route configuration for templates and redirections #24640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
javiereguiluz opened this issue Oct 20, 2017 · 13 comments
Closed

[DX][FrameworkBundle] Simpler route configuration for templates and redirections #24640

javiereguiluz opened this issue Oct 20, 2017 · 13 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature FrameworkBundle Stalled

Comments

@javiereguiluz
Copy link
Member

Context

In #23227 we made this change:

# BEFORE ----------------------------------------------------------------------
blog_show:
    path:     /blog/{slug}
    defaults: { _controller: App\Controller\BlogController::showAction }

# AFTER -----------------------------------------------------------------------
blog_show:
    path:       /blog/{slug}
    controller: App\Controller\BlogController::showAction

In #24637 we improved a bit the routes that render a template directly, but I don't think that's enough.

Proposal

Let's finish #23227 by adding shortcuts for templates and redirections:

Templates

# BEFORE ----------------------------------------------------------------------
blog_show:
    path:     /blog/{slug}
    defaults:
        _controller: Symfony\Bundle\FrameworkBundle\Controller\TemplateController
        template: blog/show.html.twig

# AFTER -----------------------------------------------------------------------
blog_show:
    path:     /blog/{slug}
    template: blog/show.html.twig

Redirections

If the value of redirect_to starts with / or // or http:// or https://, it's considered a URL redirect. Otherwise, it's a route redirect:

# BEFORE ----------------------------------------------------------------------
blog_show:
    path:     /blog/{slug}
    defaults:
        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction
        path: /some-relative-url
        permanent: true

# AFTER -----------------------------------------------------------------------
blog_show:
    path:        /blog/{slug}
    redirect_to: /some-relative-url
    permanent:   true
# BEFORE ----------------------------------------------------------------------
blog_show:
    path:     /blog/{slug}
    defaults:
        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
        route: some-other-route

# AFTER -----------------------------------------------------------------------
blog_show:
    path:        /blog/{slug}
    redirect_to: some-other-route
@javiereguiluz javiereguiluz added DX DX = Developer eXperience (anything that improves the experience of using Symfony) FrameworkBundle labels Oct 20, 2017
@chalasr
Copy link
Member

chalasr commented Oct 20, 2017

👍

1 similar comment
@Simperfit
Copy link
Contributor

👍

@javiereguiluz javiereguiluz added this to the 4.1 milestone Oct 20, 2017
@stof
Copy link
Member

stof commented Oct 20, 2017

These would require making the routing component depend on FrameworkBundle (these special controllers are part of the bundle, but route definitions are handled in the component)

@javiereguiluz
Copy link
Member Author

@stof there's no need for that. If you have router + frameworkbundle, you can use this feature. If you don't have frameworkbundle and try to use this feature, you'll see an exception. We already do that for lots of Symfony features.

@kiler129
Copy link
Contributor

This is indeed a great simplification. Huge 👍

@sroze sroze mentioned this issue Nov 24, 2017
Closed
@sroze
Copy link
Contributor

sroze commented Nov 24, 2017

Proposed a PR for the redirection in #25145

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 24, 2017 •
edited
Loading

If you don't have frameworkbundle and try to use this feature, you'll see an exception

I'm really not sure about that part. And I'm not sure we do it for any Symfony features. High-level can adapt to varying low level stuffs. But having a low level stuff (Routing) know about a high level one (FrameworkBundle) looks strange to me.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 24, 2017 •
edited
Loading

I would do it differently: instead of setting a controller, the redirect_to entry could just set a new entry in "defaults", named eg "_redirect_to" instead of "_controller". Then it'd be up to high level layers to act upon it.

@sroze
Copy link
Contributor

sroze commented Nov 24, 2017

Yep, I agree it would be a better option. Let's discuss the implementation details within the PR :)

@dunglas
Copy link
Member

dunglas commented Dec 3, 2017

Or maybe can we just pass unknown keys (or even all keys) as request's attributes. It solves all issues and provides a great extension point for new usages.

@sroze
Copy link
Contributor

sroze commented Dec 3, 2017

But this increases the risk of a bad DX because of typos and things like that... I'd prefer having a proper validation around it tbh

@javiereguiluz javiereguiluz mentioned this issue Jan 19, 2018
Closed
@nicolas-grekas nicolas-grekas removed this from the 4.1 milestone Apr 20, 2018
@HeahDude HeahDude mentioned this issue Mar 9, 2019
Merged
4 tasks
@HeahDude HeahDude mentioned this issue Feb 9, 2020
Closed
nicolas-grekas added a commit that referenced this issue Feb 9, 2020
@nicolas-grekas
feature #30501 [FrameworkBundle][Routing] added Configurators to hand…
477ee19 
…le template and redirect controllers (HeahDude)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[FrameworkBundle][Routing] added Configurators to handle template and redirect controllers

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | let's see
| Fixed tickets | partially #24640, #25145
| License       | MIT
| Doc PR        | symfony/symfony-docs#11120

While working on symfony/symfony-docs#11085, I felt bad about the long notations required for simple [redirects](https://symfony.com/doc/current/routing/redirect_in_config.html) and [templates rendering](https://symfony.com/doc/current/templating/render_without_controller.html) template actions, but I love and use those features since always. Then I gave it a try yesterday night and now I realised I missed #24640 and that #25145 has been closed x).

So here we go, here's my WIP. WDYT of this implementation? ping @javiereguiluz?

I'm going to open the PR in the docs so we can discuss the DX changes there too, and keep focus on the code here.

Cheers!

EDIT
----
This PR now only update PHP-DSL configurators.

______________

TODO:

- [x] gather reviews
- ~[x] fix xml schema~
- [x] add some tests
- ~[ ] handle xsd auto discovery~
- [x] rebase on top of #30507
- [x] ~add shortcuts for #30514~

Commits
-------

de74794 [FrameworkBundle][Routing] added Configurators to handle template and redirect controllers
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@nicolas-grekas
Copy link
Member

On my side, I think #35653 had the correct approach.
Since it's been rejected, I think we can close here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature FrameworkBundle Stalled
Projects
None yet
Milestone
No milestone
9 participants
@dunglas @javiereguiluz @nicolas-grekas @stof @sroze @kiler129 @Simperfit @chalasr @carsonbot