8000 [FrameworkBundle][Routing] added XML and YAML loaders to handle template and redirect controllers by HeahDude · Pull Request #35653 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle][Routing] added XML and YAML loaders to handle template and redirect controllers #35653

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
wants to merge 1 commit into from
Closed

[FrameworkBundle][Routing] added XML and YAML loaders to handle template and redirect controllers #35653

wants to merge 1 commit into from

Conversation

HeahDude
Copy link
Contributor
@HeahDude HeahDude commented Feb 9, 2020
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #24640
License MIT
Doc PR symfony/symfony-docs#11120

Split from #30501 to add support in XML and YAML formats.

I've also improved the XML support for context with the TemplateController introduced in #35257.

This was referenced Feb 9, 2020
Merged
Closed
@nicolas-grekas nicolas-grekas added this to the next milestone Feb 9, 2020
@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch 2 times, most recently from 46d5cbf to 370f4bf Compare February 9, 2020 23:14
@HeahDude
Copy link
Contributor Author
HeahDude commented Feb 9, 2020

PR rebased now that #30501 has been merged. Added support for gone routes too for consistency.

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch 6 times, most recently from ff6e635 to 5eb4e50 Compare February 12, 2020 18:00
@HeahDude
Copy link
Contributor Author

I've tweaked the XSD and now use an unavailable (yet) schema location https://symfony.com/schema/routing/framework-routing-1.0.xsd.
Autocomplete works as expected locally, and tests are also green locally. I guess having the file hosted is the final move to get the CI green here.

Ready for some (final?) review. Thanks!

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch 5 times, most recently from d1f6608 to ee4a389 Compare February 14, 2020 19:24
@HeahDude
Copy link
Contributor Author

I've tweaked the XmlLoader and now tests are green on appveyor too 🎉. As a bonus we now prevent some HTTP calls to load the XSDs.

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch 6 times, most recently from a0ca2a1 to 8b3ce31 Compare February 15, 2020 08:13
nicolas-grekas
nicolas-grekas approved these changes Feb 15, 2020
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch from 1aada92 to fec4cea Compare March 8, 2020 12:20
@HeahDude
Copy link
Contributor Author
HeahDude commented Mar 8, 2020

I've added a test and a proper exception when using many types in YAML (which is already enforce by PHP-DSL and XSD) for case such like:

route:
    path: /redirect
    redirect_to_route: target_route
    template: static.html.twig

The exception message might need some rewording though.

@Tobion please consider reviewing also the related docs PR where things are more simplified than ambiguous IMHO, since YAML is still the first format to show up, we do already document the controller arguments, and that requires a lot of boilerplate, hence #24640 a DX proposal.
Somehow the feature itself (including PHP-DSL already merged) is not auto discoverable unless we merge symfony/recipes#721.

Actually the gone routes were not even documented, it currently has to be rewritten as:

gone_route:
    path: /gone
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
    defaults:
        route: ''
        #permanent: true # optional

vs with this PR:

gone_route:
    path: /gone
    gone: true
    #permanent: true # optional

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch 2 times, most recently from 1345b28 to cd325e1 Compare March 8, 2020 12:54
@Tobion
Copy link
Contributor
Tobion commented Apr 1, 2020

I stand by my arguments. If the gone declaration is not so nice, it can be fixed in the redirectAction.

@nicolas-grekas
Copy link
Member

I have an opposite analysis from you @Tobion: having to know about RedirectController and TemplateController is leaking an implementation detail.

This PR makes the yaml and xml definitions more abstracts - which means more descriptive. Being more descriptive is a nice improvement as it allows decoupling the "what" and the "how".

The XSD definition is perfectly clear about what is allowed or not, there is no need to know anything about the implementation to be able to correctly describe what one wants. For Yaml, there is no schema, so your concern is not specific to this PR but to the format in general, thus it cannot be enough a reason to reject this PR on its own.

I'm definitely 👍

I would even go one step further and move these two news concepts (redirect+template routes) to the component. We would then make loaders configurable so that these can wire any redirect/templating logic.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 1, 2020 •
edited
Loading

@Tobion also, this matters:
image

I agree we don't do github-reactions-driven-development, but we can still use it to weight our interest for a PR :) The implementation proposed here is sound and fixes the linked RFC in a very sane way.

The only question to me is whether we should move all this to the component as I mentioned.

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch 3 times, most recently from 45e8b7d to bd4af21 Compare April 1, 2020 11:26
@Tobion
Copy link
Contributor
Tobion commented Apr 1, 2020

having to know about RedirectController and TemplateController is leaking an implementation detail

Making routing know about templates etc is mixing responsibilities. Why would routing need to know about rendering templates? That is violating the single responsibility principle obviously.

@Tobion
Copy link
Contributor
Tobion commented Apr 1, 2020 •
edited
Loading

Also why not also add the possiblity to define response headers here? You can render templates, do redirects etc. Why can't I add response headers?
You cannot draw the line if you start something like that and add more and more shortcuts.

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch from bd4af21 to 3dbc827 Compare April 1, 2020 11:41
@HeahDude
Copy link
Contributor Author
HeahDude commented Apr 1, 2020

It seems to me that those arguments should be for #24640, which should then be closed as won't fix or not, as I don't see how this is related to the implementation of this PR.

@nicolas-grekas
Copy link
Member

Making routing know about templates etc is mixing responsibilities. Why would routing need to know about rendering templates? That is violating the single responsibility principle obviously.

It's not making the routing component know about templates. It's also not breaking SRP. Routing still has one single responsibility: mapping a request to a set of key-value pairs.

This PR is about giving a few more ways to define which key-value pairs are mapped to which request.

Also why not also add the possiblity to define response headers here? You can render templates, do redirects etc. Why can't I add response headers?

This is misleading. This PR is not about "rendering templates", nor "do redirect". As I stated just above: this is about mapping useful input declarations to useful output key-value pairs. How a template is rendered or how a redirect is done is foreign concept.

To answer your question: "define response headers?" Not as such. But mapping an input definition to some output key-value pairs that another component/logic could use to send response headers, why not. This is not what this PR is about so let's keep the focus.

You cannot draw the line if you start something like that and add more and more shortcuts.

That's our job as OSS maintainers: identifying patterns that are common enough and find a way to move them at a more generic level.

But I'm fine keeping the extra mappings in the framework bundle. Moving them in the component is extra work we might prefer saving us from doing - getting generic is hard.

In the end, I think this is not a technical decision anymore. The question is about the documentation as you asked at the beginning. It's obvious that this PR provides greater DX to ppl that use the FrameworkBundle. What about ppl that use the component standalone? I think the general trend in the doc is now to document the things integrated first, and consider that ppl that use things standalone are advanced enough to figure out the rest by themselves - or by "advanced" articles. As long as the components remain standalone and useful on their own, I think this is the right thing do to.

@HeahDude HeahDude force-pushed the framework-xml-yaml-routing branch from 3dbc827 to 334434f Compare April 12, 2020 12:45
@Tobion
Copy link
Contributor
Tobion commented Apr 19, 2020 •
edited
Loading

Current example:

template_route:
    path: /static
    template: static.html.twig
    context:
        foo: bar
    max_age: 300

The whole thing is not bound a specific controller anymore (which defined maxAge params etc.), but meant to be a generic solution that allows more mapping options from request to response as Nicolas explained. With that in mind, I don't think limiting this to a few response headers like max-age is the right approach. Instead I'd suggest to add a generic response headers option. This has several advantages:

  1. it's not only bound to templates but also works for redirects etc.
  2. There is no naming inconsistencies (max_age in yaml vs max-age in xml)
  3. Way more useful: If you render a template using this route config but want to add a new response header, what do you do? Either you need to write a whole custom controller again or add some kind of custom listener. So it defeats the whole purpose of having a simple way to render a template as soon as you need a simple modification like a new header.
  4. It also way more useful if you use something like a view transformer as you can add a response header without having to render the response manually in the controller again, see Add Headers to response FriendsOfSymfony/FOSRestBundle#1888

@nicolas-grekas nicolas-grekas mentioned this pull request Apr 24, 2020
Merged
fabpot added a commit that referenced this pull request Apr 24, 2020
@fabpot
feature #36562 Revert "feature #30501 [FrameworkBundle][Routing] adde…
c6cf433 
…d Configurators to handle template and redirect controllers (HeahDude)" (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

Revert "feature #30501 [FrameworkBundle][Routing] added Configurators to handle template and redirect controllers (HeahDude)"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License    
8000
   | MIT
| Doc PR        | -

This reverts commit 477ee19, reversing changes made to 9bfa258.

The reverted PR is #30501. Actually, it's a partial revert: changes made to the `Routing` component are not reverted.

The reason for this revert is that a discussion is still on-going in #35653 (which provides the same configuration extensions to yaml+xml), and we won't be able to resolve them in time for 5.1.

Better postpone for 5.2.

@HeahDude I invite you to open a PR after this one, reverting this very PR, which we'll consider again but for 5.2 if you don't mind.

Commits
-------

e4e8945 Revert "feature #30501 [FrameworkBundle][Routing] added Configurators to handle template and redirect controllers (HeahDude)"
@fabpot
Copy link
Member
fabpot commented Aug 17, 2020

Closing as there is no consensus.

@fabpot fabpot closed this Aug 17, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

@Tobion Tobion Tobion requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature FrameworkBundle Routing Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

[DX][FrameworkBundle] Simpler route configuration for templates and redirections
6 participants
@HeahDude @Tobion @nicolas-grekas @fabpot @weaverryan @carsonbot
0