Skip to content

[FrameworkBundle][DX] Improving the redirect config when using RedirectController #33217

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
Merged
merged 1 commit into from
Aug 23, 2019
Merged

[FrameworkBundle][DX] Improving the redirect config when using RedirectController #33217

merged 1 commit into from
Aug 23, 2019

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Aug 16, 2019 •
edited
Loading

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

follow-up #24637

Before:

# config/routes.yaml
doc_shortcut:
    path: /doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
    defaults:
        route: 'doc_page'

legacy_doc:
    path: /legacy/doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction
    defaults:
        path: 'https://legacy.example.com/doc'

After:

# config/routes.yaml
doc_shortcut:
    path: /doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController
    defaults:
        route: 'doc_page'

legacy_doc:
    path: /legacy/doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController
    defaults:
        path: 'https://legacy.example.com/doc'

See more before/after configs (XML, PHP) in doc PR symfony/symfony-docs#12189

@carsonbot carsonbot added Status: Needs Review FrameworkBundle DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Aug 16, 2019
@yceruto yceruto mentioned this pull request Aug 16, 2019
Merged
@yceruto yceruto force-pushed the simpler_redirect_config branch from 6ce11cc to 93e69ea Compare August 16, 2019 21:07
@yceruto yceruto added this to the next milestone Aug 16, 2019
@yceruto yceruto force-pushed the simpler_redirect_config branch from 93e69ea to bea281d Compare August 17, 2019 03:43
@wouterj
Copy link
Member

wouterj commented Aug 17, 2019

👍 What about deprecating the other two methods? (in favor of making them private/protected in Symfony 5.0)

@nicolas-grekas
Copy link
Member

I don't think deprecating is worth the trouble it will put on users.

@linaori
Copy link
Contributor

linaori commented Aug 17, 2019

From a DX perspective, this would be really amazing if we could make something like this:

# config/routes.yaml
doc_shortcut:
    path: /doc
    controller: RedirectController
    defaults:
        route: 'doc_page'

legacy_doc:
    path: /legacy/doc
    controller: RedirectController
    defaults:
        path: 'https://legacy.example.com/doc'

Or perhaps:

# config/routes.yaml
doc_shortcut:
    path: /doc
    redirect:
        route: 'doc_page'

legacy_doc:
    path: /legacy/doc
    redirect:
        path: 'https://legacy.example.com/doc'

nicolas-grekas
nicolas-grekas reviewed Aug 18, 2019
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.

We should throw also when both settings are defined at the same time.

@yceruto yceruto force-pushed the simpler_redirect_config branch from bea281d to deada37 Compare August 19, 2019 11:20
stof
stof reviewed Aug 20, 2019
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php
throw new \RuntimeException('Ambiguous redirection settings, use the "path" or "route" parameter, not both.');
}

return $this->redirectAction($request, $p['route'], $p['permanent'] ?? false, $p['ignoreAttributes'] ?? false, $p['keepRequestMethod'] ?? false, $p['keepQueryParams'] ?? false);
Copy link
Member

Choose a reason for hiding this comment

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

for all the common parameters, you could make them arguments of the controller instead, to let Symfony bind them (with any logic applied by argument resolver, as done for other methods) and take the default values from the method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it's worth it just for two common parameters.

@yceruto yceruto force-pushed the simpler_redirect_config branch from 2ee8baf to 33fd9b0 Compare August 20, 2019 19:41
@yceruto yceruto force-pushed the simpler_redirect_config branch from 33fd9b0 to cb3cafa Compare August 20, 2019 19:56
@yceruto yceruto force-pushed the simpler_redirect_config branch from cb3cafa to 0ebb469 Compare August 23, 2019 21:46
yceruto added a commit that referenced this pull request Aug 23, 2019
@yceruto
feature #33217 [FrameworkBundle][DX] Improving the redirect config wh…
dc8d470 
…en using RedirectController (yceruto)

This PR was merged into the 4.4 branch.

Discussion
----------

[FrameworkBundle][DX] Improving the redirect config when using RedirectController

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

follow-up #24637

**Before:**
```yaml
# config/routes.yaml
doc_shortcut:
    path: /doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
    defaults:
        route: 'doc_page'

legacy_doc:
    path: /legacy/doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction
    defaults:
        path: 'https://legacy.example.com/doc'
```

**After:**
```yaml
# config/routes.yaml
doc_shortcut:
    path: /doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController
    defaults:
        route: 'doc_page'

legacy_doc:
    path: /legacy/doc
    controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController
    defaults:
        path: 'https://legacy.example.com/doc'
```

See more before/after configs (XML, PHP) in doc PR symfony/symfony-docs#12189

Commits
-------

0ebb469 Improving redirect config when using RedirectController
@yceruto yceruto merged commit 0ebb469 into symfony:4.4 Aug 23, 2019
@yceruto yceruto deleted the simpler_redirect_config branch August 23, 2019 21:48
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Sep 2, 2019
@javiereguiluz
minor #12189 [FrameworkBundle][DX] Improving redirect config when usi…
19e9d0b 
…ng RedirectController (yceruto)

This PR was merged into the 4.4 branch.

Discussion
----------

[FrameworkBundle][DX] Improving redirect config when using RedirectController

Update according to symfony/symfony#33217

Commits
-------

f59c61f [DX] Improving redirect config
@nicolas-grekas nicolas-grekas removed this from the next milestone Oct 27, 2019
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

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

@Tobion Tobion Tobion approved these changes

Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@yceruto @wouterj @nicolas-grekas @linaori @stof @Tobion @carsonbot