Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[FrameworkBundle] Improve DX of RedirectController#25259

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
dunglas wants to merge1 commit intosymfony:masterfromdunglas:dx-controller

Conversation

@dunglas
Copy link
Member

@dunglasdunglas commentedDec 1, 2017•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#...
LicenseMIT
Doc PRtodo

Similar to#24637 but for redirect controller.

TODO:

  • Add tests

@dunglas
Copy link
MemberAuthor

Status: needs review

@ro0NL
Copy link
Contributor

isnt adding a single test to cover the method forwarding simpler?

@dunglas
Copy link
MemberAuthor

Well doing that allowed me to find some bugs in my forwarding logic. So I would prefer to keep al the tests.

@sroze
Copy link
Contributor

Maybe that's not that useful if we go with this PR that allows redirection to be set directly in the routing configuration?
#25145

@dunglas
Copy link
MemberAuthor

@sroze IMO we must have an invoke method for all builtin controllers.

ro0NL and jvasseur reacted with thumbs up emoji


publicfunction__invoke(Request$request):Response
{
if (null !==$route =$request->attributes->get('route')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like alot of magic. It might be cleaner to just create a new controller with invoke for one action. And possibly deprecate it in RedirectController so there is no ambiguity.

xabbuh, HeahDude, and pizzaminded reacted with thumbs up emoji
fabpot added a commit that referenced this pull requestJan 19, 2018
This PR was merged into the 4.1-dev branch.Discussion----------[DI] Allow for invokable event listeners| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->Inspired by#24637 /#25259. This adds invokable support for event listeners :)```yamlSome\Foo:    tags: [{ name: kernel.event_listener, event: kernel.request }]``````phpclass Foo {    public function __invoke(GetResponseEvent $event) { }}```Commits-------fa5b7eb [DI] Allow for invokable event listeners
@nicolas-grekas
Copy link
Member

I'm not convinced this provides anything. Actually, I'm much more convinced by the aliasing discussed in#25145 (comment)

👎

@fabpot
Copy link
Member

I don't see why wemust provide an__invoke method for all built-in controllers. I think DX would be much more improved if we would not have to use the FQCN instead.
👎

@nicolas-grekasnicolas-grekas modified the milestones:4.1,nextApr 20, 2018
@nicolas-grekasnicolas-grekas modified the milestones:next,4.1Nov 1, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

2 more reviewers

@TobionTobionTobion left review comments

@SimperfitSimperfitSimperfit approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

8 participants

@dunglas@ro0NL@sroze@nicolas-grekas@fabpot@Tobion@Simperfit@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp