Skip to content

Update book to comply with best practices, round 3#4779

Merged
weaverryan merged 5 commits intosymfony:2.3from
wouterj:best_practices_3
Mar 13, 2015
Merged

Update book to comply with best practices, round 3#4779
weaverryan merged 5 commits intosymfony:2.3from
wouterj:best_practices_3

Conversation

@wouterj
Copy link
Copy Markdown
Member

@wouterj wouterj commented Jan 6, 2015

Q A
Doc fix? yes
New docs? yes
Applies to all
Fixed tickets #4431

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I believe this was the guideline, but it was never written somewhere. As lots of people are struggling with it, I think it's a good idea to document it. While doing this, I've also snake_cased all template paths in the currently updated book articles.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@javiereguiluz What do you think? We've never talked too much about this directly. The negative to underscoring everything is that templates aren't the same as their controllers anymore - PostAdminController::createPostAction would be in post_admin/create_post.html.twig.

We're just talking about a recommendation here - not life or death - but let's think about it. The old upper-camel case was done without really thinking, to support the @Template magic (because we were all very used to having this from 1.x). If we break from this, then @Template won't work, and even if we update it, trying to transform from createPostAction to create_post.html.twig could be buggy.

What do guys think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this moment, my thinking is that the old way seems the best, as it's very simple: everything just matches: no transformations (of course, you can do whatever you want).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ping @javiereguiluz I need your input on upper/lower case template directories here - this will affect a lot of different parts. Thanks :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ping again @javiereguiluz!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

moved these changes to #5001

@wouterj
Copy link
Copy Markdown
Member Author

wouterj commented Jan 6, 2015

While changing the Testing and HttpCache articles, I got a bit distracted fixing other things :) I'll do a follow up PR of a Testing article rewrite, as it isn't really nicely written (and it's the oldest not actively maintained document).

@stof
Copy link
Copy Markdown
Member

stof commented Jan 6, 2015

This will conflict with #4740

@wouterj
Copy link
Copy Markdown
Member Author

wouterj commented Jan 6, 2015

I don't think so, as Javier updated the cookbook and I the book

@stof
Copy link
Copy Markdown
Member

stof commented Jan 6, 2015

ah, I forgot the fact that his PR does not do the search and replace on the whole doc.

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not correct, see #4789 (including some other fixes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the issue? Just that we should have the line breaks different?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

missing new

@wouterj wouterj mentioned this pull request Jan 11, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be something like default/homepage.html.twig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i would prefer valid php here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is our standard

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants