Skip to content
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.

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

[FrameworkBundle][Routing] added proper Loader namespace to handle controllers #30501

Open
wants to merge 1 commit into
base: master
from

Conversation

@HeahDude
Copy link
Member

HeahDude commented Mar 9, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? let's see
Fixed tickets #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 and templates rendering 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!


TODO:

  • gather reviews
  • fix xml schema
  • add some tests
  • handle xsd auto discovery
  • rebase on top of #30507
  • add shortcuts for #30514
@HeahDude

This comment has been minimized.

Copy link
Member Author

HeahDude commented Mar 9, 2019

I'd really appreciate some help if anyone had already overridden some xsd schema thanks!

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 9, 2019
@HeahDude HeahDude force-pushed the HeahDude:framework-routing branch from a8de471 to 5fa4980 Mar 10, 2019
@HeahDude

This comment has been minimized.

Copy link
Member Author

HeahDude commented Mar 10, 2019

I've updated symfony/symfony-docs#11120 with a nice before/after view.

Also I need to rebase on top of #30507.

Xml config is working properly, but the IDE cannot resolve validation/autocompletion yet, this works thanks to the loader using its constant local path.
I guess we need to create a new path on https://symfony.com/routing that we need to declare explicitly in config/routes.xml or any xml file where this feature should be used, are you the one handling this @fabpot? Can you help me on this?

Apart this last points, the feature is ready from side, only reviews needed now :), thanks!

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 11, 2019

Thanks for working on this. To make this feature completely self-explanatory, I think we should rename redirect-to as redirect-to-route (we already have redirect-to-url). What do you think?

fabpot added a commit that referenced this pull request Mar 11, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

[Routing] Fixed XML options resolution

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Found this bug while adding tests in #30501. I need it to be merged upward so it can get green there.
Thanks!

Commits
-------

53a6ff8 [Routing] Fixed XML options resolution
@HeahDude HeahDude force-pushed the HeahDude:framework-routing branch from e251bae to a46c76a Mar 11, 2019
@HeahDude HeahDude changed the title [FrameworkBundle][Routing] added a RoutingConfigurator to handle template and redirect [FrameworkBundle][Routing] added proper Loader namespace to handle controllers Mar 13, 2019
fabpot added a commit that referenced this pull request Mar 20, 2019
… "format" in configuration (Jules Pietri)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Routing] Exposed "utf8" option, defaults "locale" and "format" in configuration

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | symfony/symfony-docs#11126

A sibling to #30501, everything is in the title :).

Commits
-------

2911490 [Routing] Exposed "utf8" option, defaults "locale" and "format" in configuration
@HeahDude HeahDude force-pushed the HeahDude:framework-routing branch from a46c76a to 7055ba0 Mar 21, 2019
@HeahDude

This comment has been minimized.

Copy link
Member Author

HeahDude commented Mar 21, 2019

Rebased and squashed here, while I addressed @javiereguiluz's comment.

Also, I was not happy with the xml definition because all attributes are there and it's hard to guess which one to use in which context, so I propose to use dedicated types instead: https://github.com/symfony/symfony/compare/a46c76a8c3a05adb7f984ea4d9cffbd3a446f3f5..7055ba0a5b09cec84d88f5c0447e79b65aa48a85#diff-96d406b9b38189be981bbea1bad334bd.

Tests are passing locally and on travis, but not on windows.. I wonder how this line https://github.com/symfony/symfony/pull/30501/files#diff-44f544990841fe196e4ea5c76dcc18dfR8 can be a problem, if anyone has a clue on that topic that would be awesome.

@HeahDude HeahDude force-pushed the HeahDude:framework-routing branch 3 times, most recently from cfc48bc to 8762eba Dec 8, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 8, 2019

Did you consider this instead for the PHP-DSL?

    $routes->add('template_route', '/static')
        ->template('static.html.twig')
        ->maxAge(300)
        ->sharedMaxAge(100)
        ->private(true)
        ->methods(['GET'])
        ->options(['utf8' => true])
        ->condition('abc')
    ;
    $routes->add('redirect_route', '/redirect')
        ->redirectToRoute('target_route')
        ->permanent(true)
        ->ignoreAttributes(['attr', 'ibutes'])
        ->keepRequestMethod(true)
        ->keepQueryParams(true)
        ->schemes(['http'])
        ->host('legacy')
        ->options(['utf8' => true])
    ;
    $routes->add('url_redirect_route', '/redirect-url')
        ->redirectToUrl('/url-target')
        ->permanent(true)
        ->scheme('http')
        ->httpPort(1)
        ->httpsPort(2)
        ->keepRequestMethod(true)
        ->host('legacy')
        ->options(['utf8' => true])
protected function validate($config, $name, $path)
{
// remove invalid keys for values normalized below
foreach (array_unique(array_merge(...array_values(self::$availableKeys))) as $key) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 8, 2019

Member

can we validate the attributes per type of route?
can we validate that the routes using one and only one of the 3 possibles types?

This comment has been minimized.

Copy link
@HeahDude

HeahDude Dec 8, 2019

Author Member

When I look at how the parent is implemented and extended, the only valuable thing I can think of checking here is about defining both "controller" and one of the three types we introduce.

@HeahDude HeahDude force-pushed the HeahDude:framework-routing branch 2 times, most recently from c67da45 to db3fdfd Dec 10, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 17, 2019

Now that #34873 is merged, can you please rebase and take it into account to inject your new ContainerConfigurator?

@HeahDude HeahDude force-pushed the HeahDude:framework-routing branch from db3fdfd to cda6dc7 Dec 18, 2019
@HeahDude

This comment has been minimized.

Copy link
Member Author

HeahDude commented Dec 18, 2019

Done, rebased and squashed.

/**
* @param string $template The template name
*/
final public function template(string $template): TemplateRouteConfigurator

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 8, 2020

Member

will need to account for #35257 :)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 10, 2020

Member

#35257 merged, can be rebased now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.