Skip to content

Completely re-reading the controller chapter#4433

Merged
weaverryan merged 4 commits into2.3from
best-practice-controller-updates
Nov 25, 2014
Merged

Completely re-reading the controller chapter#4433
weaverryan merged 4 commits into2.3from
best-practice-controller-updates

Conversation

@weaverryan
Copy link
Copy Markdown
Contributor

Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed ticket n/a

Hi guys!

This follows #4427, but only for the controller chapter (I will open separate PR's for from_flat_php_to_symfony2, routing, and templating).

This is more than just updating for the best practices. This is about face-lifting these important chapters after several years of us as a community realizing what's more important and what's less important. More notes:

  • I removed some information about using the defaults to pass additional information to your controller from this chapter. This is too early and we have this now: http://symfony.com/doc/current/cookbook/routing/extra_information.html
  • I removed ContainerAware information. This is not the right spot... and maybe nothing is. I don't see much use-case for this - it seems like you'd either extend the base Controller or register your controller as a service.
  • I kind of want to remove or move the forwarding stuff. I can't find a real use-case for doing a forward from one controller to another. It adds a lot of overhead and I don't see any situation for it.

Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You have to remove the underscore after the doc role.

@wouterj
Copy link
Copy Markdown
Member

wouterj commented Nov 7, 2014

I removed ContainerAware information. This is not the right spot... and maybe nothing is. I don't see much use-case for this - it seems like you'd either extend the base Controller or register your controller as a service.

+1, I was about doing the same in my PR :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sentence doesn't sound complete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you're right!

@wouterj
Copy link
Copy Markdown
Member

wouterj commented Nov 20, 2014

let's postpone this to post-2.6 :)

@weaverryan
Copy link
Copy Markdown
Contributor Author

@wouterj while not strictly related to 2.6, I'd like to have things like this done as close to 2.6 launch as possible. There is a good amount of work, but I don't want to postpone and drag it on.

Now, that would require me finding time of course. So, I'm not saying you're wrong, I'm more saying that I want to do this kind of stuff before 2.6 ;).

Thanks!

@wouterj
Copy link
Copy Markdown
Member

wouterj commented Nov 20, 2014

@weaverryan I predict that I'm not able to review this PR before 2.6, given the amount of work that has more priority (best practices update, missing 2.6 features, sf installer, etc.) That was my reason to say I would like to postpone this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants