Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.2k
Update most important book articles to follow the best practices#4427
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -967,18 +964,18 @@ see :doc:`/cookbook/routing/extra_information`. | |||
| Including External Routing Resources | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The best practices say we should use annotations. Should we include them in all routing examples? Also, is this section still relevant...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 to including annotation examples, and as the first tab. And although I want to put annotations at the main path, Ido 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 for the annotation routing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, and we will have to adopt this in the contribution docs as well.
@wouterj You are my absolute favorite for doing this! This issuch 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! |
@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! |
| @@ -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: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you plan to refer to this at a later time?
@wouterj Wow, this must have been much work. |
@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 There are also 2 another things with annotations;
I've also taken@xabbuh's suggestions into account. |
| .. code-block:: yaml | ||
| # app/config/routing.yml | ||
| homepage: | ||
| path: /{culture} | ||
| defaults: { _controller: AcmeDemoBundle:Main:homepage, culture: en } | ||
| path: /{_locale} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
was there a specific reason to call thisculture instead of_locale?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 error9678b61 Proofread templating51773f4 Proofread routing article91c8981 Used annotations for routing210bb4b Some minor things4da1bcf Use AppBundle10b3c7c Other random fixesa8a75d7 Changed Bundle:Controller:Template to bundle:dir:filedb3086d Moved templates to app and controllers to AppBundleab2e2c7 Other fixes18e3a31 Moved templates to appc8f5ad5 Updated to use AppBundleb735642 Fixed some other minor stuff0ef1c68 Moved templates to app/Resources/views87b324a Renamed AcmeHelloBundle to AppBundle
I'm proofing this right now... a second PR is on its way soon. Thanks Wouter! |
| @@ -34,12 +34,31 @@ The route is simple: | |||
| .. configuration-block:: | |||
| .. code-block:: php-annotations | |||
| // src/AppBundle/Controller/BlogController.php | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should mention that the file must be imported in the main routing file. (Or is it automatic ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we explain it later in the article. And the new SE comes with this automatically
| | /es | *won't match this route* | | ||
| +-----+--------------------------+ | ||
| ======= ======================== | ||
| path parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should these table headers be title-cased (Path andParameters) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, we agreed to do that in a separate pull request.#4435 should fix this.
Applied tweaks suggested by@javiereguiluz.
I opened a pull request with all tweaks suggested by@javiereguiluz (see#4447). |
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
* 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:: bashConflicts:quick_tour/the_big_picture.rst
* 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:: bashConflicts:quick_tour/the_big_picture.rst
* 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/aHi 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 errorceb7b94 Big update based on feedback from xabbuh and WouterJ6ef10db Moving forwarding section all the way to the bottom0754efc Completely re-reading the controller chapter
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 :)