Update most important book articles to follow the best practices #4427

wouterj · 2014-11-06T12:56:54Z

Q	A
Doc fix?	yes
New docs?	no
Applies to	all (or 2.6?)
Fixed ticket	lots of complaints :)

I've decided to do only the most important articles for the first PR, otherwise the diff will be too big to get a nice review. I'll do a seperate PR for the Page Creation article, since that needed quite so rewriting.

Btw, I also couldn't resist to fix other minor things :)

wouterj · 2014-11-06T12:59:04Z

book/routing.rst

@@ -967,18 +964,18 @@ see :doc:`/cookbook/routing/extra_information`.
 Including External Routing Resources


The best practices say we should use annotations. Should we include them in all routing examples? Also, is this section still relevant...?

/cc @weaverryan @javiereguiluz

+1 to including annotation examples, and as the first tab. And although I want to put annotations at the main path, I do want to put in a little bit of effort to highlight a bit that YAML is also a common method (just because some percentage hate annotations, so I don't want someone to think that we're forcing this on you).

Is this section relevant? I'd have to read it in context, but I think it still is (though probably less important, so it might be further down or even in another entry).

👍 for the annotation routing

Ah, and we will have to adopt this in the contribution docs as well.

weaverryan · 2014-11-06T15:30:22Z

@wouterj You are my absolute favorite for doing this! This is such an important thing to get done over the next few weeks. So yes! Let's get this merged as quickly as possible and then we can move onto other sections. Basically, you can move onto any section you want. By the end of Nov, we'll need to have covered basically everything important.

And also, this is of course a great time to fix other errors and update outdated stuff. After all, in a lot of cases, things were written years ago and may contain recommendations that have turned out to not be perfect or talk about features that turned out to not really be all that important.

Cheers!

weaverryan · 2014-11-06T15:31:06Z

@wouterj So, tell me when you're happy enough with this and I'll review it. I'll actually probably just merge it, then do a read-through and open up another PR for us to review once again (you going through this first will save a lot of time in that process).

Thanks!

xabbuh · 2014-11-06T20:38:29Z

best_practices/templates.rst

@@ -22,6 +22,8 @@ In addition, Twig is the only template format with guaranteed support in Symfony
 3.0. As a matter of fact, PHP may be removed from the officially supported
 template engines.

+.. _best_practices-template-location:


Do you plan to refer to this at a later time?

xabbuh · 2014-11-06T21:00:40Z

@wouterj Wow, this must have been much work.

wouterj · 2014-11-07T10:14:54Z

@weaverryan I added the annotations, which was a lot trickier than I thought it would be. There is one thing I don't like: The annotations remove the nice seperation of the internal layers. Before we clearly had: First the route is matched, then the _controller parameter is read by a class and then the controller is called. With annotations, this is now all mixed together and we can't really be clear about this.

There are also 2 another things with annotations;

Should we always include a route name? If not, we should add a new section on routing naming
Defaults can be done in 2 ways: @Route("/blog/{page}", defaults={"page": 1}) or $page = 1 in the controller. I like the second one more, but that means we'll be inconsistent with other formats which will lead to confusion.

I've also taken @xabbuh's suggestions into account.

wouterj · 2014-11-07T10:18:54Z

book/routing.rst

    .. code-block:: yaml

        # app/config/routing.yml
        homepage:
-            path:      /{culture}
-            defaults:  { _controller: AcmeDemoBundle:Main:homepage, culture: en }
+            path:      /{_locale}


was there a specific reason to call this culture instead of _locale?

I don't remember. I can only imagine I was trying to avoid the special behavior that _locale has. For example, if you're handling locales, you should kind of use $request->getLocale() in your controller to get it I think (in case it's being set somewhere other than your route).

In common practice, I don't think that's a big issue. I think we should keep your new changes. But, maybe we can add a quick note somewhere with a link to learn more about this special _locale. What do you think?

It's already in the book, talking about all special variables

…practices (WouterJ) This PR was merged into the 2.3 branch. Discussion ---------- Update most important book articles to follow the best practices | Q | A | --- | --- | Doc fix? | yes | New docs? | no | Applies to | all (or 2.6?) | Fixed ticket | lots of complaints :) I've decided to do only the most important articles for the first PR, otherwise the diff will be too big to get a nice review. I'll do a seperate PR for the Page Creation article, since that needed quite so rewriting. Btw, I also couldn't resist to fix other minor things :) Commits ------- e56c272 Fixed error 9678b61 Proofread templating 51773f4 Proofread routing article 91c8981 Used annotations for routing 210bb4b Some minor things 4da1bcf Use AppBundle 10b3c7c Other random fixes a8a75d7 Changed Bundle:Controller:Template to bundle:dir:file db3086d Moved templates to app and controllers to AppBundle ab2e2c7 Other fixes 18e3a31 Moved templates to app c8f5ad5 Updated to use AppBundle b735642 Fixed some other minor stuff 0ef1c68 Moved templates to app/Resources/views 87b324a Renamed AcmeHelloBundle to AppBundle

weaverryan · 2014-11-07T15:43:25Z

I'm proofing this right now... a second PR is on its way soon. Thanks Wouter!

GromNaN · 2014-11-07T17:11:27Z

book/routing.rst

@@ -34,12 +34,31 @@ The route is simple:

 .. configuration-block::

+    .. code-block:: php-annotations
+
+        // src/AppBundle/Controller/BlogController.php


You should mention that the file must be imported in the main routing file. (Or is it automatic ?)

we explain it later in the article. And the new SE comes with this automatically

javiereguiluz · 2014-11-11T16:34:03Z

book/routing.rst

-| /es | *won't match this route* |
-+-----+--------------------------+
+======= ========================
+path    parameters


Should these table headers be title-cased (Path and Parameters) ?

Yes, we agreed to do that in a separate pull request. #4435 should fix this.

@javiereguiluz

Applied tweaks suggested by @javiereguiluz.

xabbuh · 2014-11-11T17:50:53Z

I opened a pull request with all tweaks suggested by @javiereguiluz (see #4447).

@javiereguiluz

This PR was merged into the 2.3 branch. Discussion ---------- [Book] tweaks to #4427 | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | all | Fixed tickets | Applied tweaks suggested by @javiereguiluz. Commits ------- caf927f tweaks to #4427

@xabbuh

* 2.3: [#4336] Backporting minor syntax fix Included a bunch of fixes suggested by the awesome Symfony doc reviewers Added a bunch of fixes suggested by @xabbuh Fixed a RST formatting issue Revamped the Quick Start tutorial tweaks to #4427 Updated first code-block:: bash Conflicts: quick_tour/the_big_picture.rst

@xabbuh

* 2.5: [#4336] Backporting minor syntax fix Included a bunch of fixes suggested by the awesome Symfony doc reviewers Added a bunch of fixes suggested by @xabbuh Fixed a RST formatting issue Revamped the Quick Start tutorial tweaks to #4427 Updated first code-block:: bash Conflicts: quick_tour/the_big_picture.rst

@xabbuh

* 2.7: typos in the var-dumper component [#4243] Tweaks to the new var-dumper component [Form] Add entity manager instance support for em option [#4336] Backporting minor syntax fix Included a bunch of fixes suggested by the awesome Symfony doc reviewers Added a bunch of fixes suggested by @xabbuh Fixed a RST formatting issue Revamped the Quick Start tutorial tweaks to #4427 Updated first code-block:: bash

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

wouterj added 11 commits November 5, 2014 22:34

Renamed AcmeHelloBundle to AppBundle

87b324a

Moved templates to app/Resources/views

0ef1c68

Fixed some other minor stuff

b735642

Updated to use AppBundle

c8f5ad5

Moved templates to app

18e3a31

Other fixes

ab2e2c7

Moved templates to app and controllers to AppBundle

db3086d

Changed Bundle:Controller:Template to bundle:dir:file

a8a75d7

Other random fixes

10b3c7c

Use AppBundle

4da1bcf

Some minor things

210bb4b

wouterj reviewed Nov 6, 2014
View reviewed changes

wouterj added the Status: Needs Work label Nov 6, 2014

xabbuh reviewed Nov 6, 2014
View reviewed changes

wouterj added 4 commits November 7, 2014 10:47

Used annotations for routing

91c8981

Proofread routing article

51773f4

Proofread templating

9678b61

Fixed error

e56c272

wouterj reviewed Nov 7, 2014
View reviewed changes

wouterj added Status: Reviewed and removed Status: Needs Work labels Nov 7, 2014

wouterj mentioned this pull request Nov 7, 2014

Update book to comply with best practices #4431

Closed

13 tasks

weaverryan merged commit e56c272 into symfony:2.3 Nov 7, 2014

wouterj deleted the update_best_practice branch November 7, 2014 15:40

weaverryan mentioned this pull request Nov 7, 2014

Completely re-reading the controller chapter #4433

Merged

GromNaN reviewed Nov 7, 2014
View reviewed changes

xabbuh mentioned this pull request Nov 7, 2014

remove unused label #4434

Merged

javiereguiluz reviewed Nov 11, 2014
View reviewed changes

xabbuh added a commit to xabbuh/symfony-docs that referenced this pull request Nov 11, 2014

tweaks to symfony#4427

caf927f

Applied tweaks suggested by @javiereguiluz.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update most important book articles to follow the best practices #4427

Update most important book articles to follow the best practices #4427

wouterj commented Nov 6, 2014

wouterj Nov 6, 2014

weaverryan Nov 6, 2014

xabbuh Nov 6, 2014

xabbuh Nov 6, 2014

weaverryan commented Nov 6, 2014

weaverryan commented Nov 6, 2014

xabbuh Nov 6, 2014

xabbuh commented Nov 6, 2014

wouterj commented Nov 7, 2014

wouterj Nov 7, 2014

weaverryan Nov 7, 2014

wouterj Nov 7, 2014

weaverryan commented Nov 7, 2014

GromNaN Nov 7, 2014

wouterj Nov 7, 2014

javiereguiluz Nov 11, 2014

xabbuh Nov 11, 2014

xabbuh commented Nov 11, 2014

		@@ -967,18 +964,18 @@ see :doc:`/cookbook/routing/extra_information`.
		Including External Routing Resources

Update most important book articles to follow the best practices #4427

Update most important book articles to follow the best practices #4427

Conversation

wouterj commented Nov 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Nov 6, 2014

weaverryan commented Nov 6, 2014

Choose a reason for hiding this comment

xabbuh commented Nov 6, 2014

wouterj commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Nov 11, 2014