Skip to content

Completely re-reading the controller chapter #4433

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

Merged
merged 4 commits into from
Nov 25, 2014

Conversation

weaverryan
Copy link
Member

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!

class called ``HelloController`` inside a bundle named ``AppBundle``. The
method ``indexAction()`` is then executed.
Now, you can go to ``/hello/ryan`` (e.g. ``http://localhost:8000/app_dev.php/hello/ryan``
if you're using the :doc:`built-in web server </cookbook/webserver/built_in>`_)
Copy link
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
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 :)

@@ -762,22 +747,26 @@ headers and content that's sent back to the client::
$response = new Response(json_encode(array('name' => $name)));
$response->headers->set('Content-Type', 'application/json');

.. tip::
The ``headers`` property is a :class:`Symfony\\Component\\HttpFoundation\\HeaderBag`
object some nice methods for getting and setting the headers. The header
Copy link
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
Member 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
Member

wouterj commented Nov 20, 2014

let's postpone this to post-2.6 :)

@weaverryan
Copy link
Member 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
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 :)

@weaverryan
Copy link
Member Author

@wouterj I understand! Do your best, but of course we're all just giving our free time :).

And thanks for the feedback - I've just pushed updates based on everything. The only pending issue I know of is this one: #4433 (comment). Wouter - have you reviewed this entire PR when you originally added comments, or only part of it?

Thanks!

@wouterj
Copy link
Member

wouterj commented Nov 24, 2014

Wouter - have you reviewed this entire PR when you originally added comments, or only part of it?

I don't believe I did, although my comments reached from start to end... We don't have to stop this PR from merging though, I can always create a follow up.

@weaverryan
Copy link
Member Author

@wouterj Good plan - I think we have this most right. I'll get the tests passing and get this guy merged in. Thanks :)

controller and passes in ``ryan`` for the ``$name`` variable. Creating a
"page" means simply creating a controller method and associated route.
Now, you can go to ``/hello/ryan`` (e.g. ``http://localhost:8000/app_dev.php/hello/ryan``
if you're using the :doc:`built-in web server </cookbook/webserver/built_in>`)
Copy link
Member

Choose a reason for hiding this comment

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

You are missing the underscore here (web_server instead of webserver).

@weaverryan
Copy link
Member Author

I've merged this in - thanks for the help! Let's keep moving forward with 2.6 updates and re-reading parts of the docs to have more updates like this. Yes!

@weaverryan weaverryan merged commit 903f52c into 2.3 Nov 25, 2014
weaverryan added a commit that referenced this pull request Nov 25, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

Completely re-reading the controller chapter

| 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!

Commits
-------

903f52c Fixing build error
ceb7b94 Big update based on feedback from xabbuh and WouterJ
6ef10db Moving forwarding section all the way to the bottom
0754efc Completely re-reading the controller chapter
@xabbuh xabbuh deleted the best-practice-controller-updates branch December 11, 2014 18:32
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