Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update security.rst #6637

Closed
wants to merge 2 commits into from
Closed

Update security.rst #6637

wants to merge 2 commits into from

Conversation

norbert-n
Copy link
Contributor

missing use statement

@OskarStark
Copy link
Contributor

LGTM 👍

xabbuh
xabbuh reviewed Jun 8, 2016
View reviewed changes
book/security.rst
@@ -185,6 +185,7 @@ example, if you use annotations, create something like this::

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please sort the use statements alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@javiereguiluz
Copy link
Member

It looks good ... but I don't know if doc maintainers will agree with it. The // ... at the beginning of the code is used in the Symfony Docs to refer to "all the "use" that are needed but we don't show them here because they are very basic or they were showed in the previous examples".

@wouterj
Copy link
Member

wouterj commented Jun 10, 2016

Personally, I only apply // ... to the "shown before" standard, not the "very basic". For beginners, nothing is basic in Symfony. So a 👍 for me.

@xabbuh
Copy link
Member

xabbuh commented Jun 11, 2016

I absolutely agree with @wouterj. This is the book which is probably one of the first parts newcomers are reading. And secondly this is the first controller example in this chapter. So 👍 from me too.

xabbuh added a commit that referenced this pull request Jun 11, 2016
@xabbuh
minor #6637 Update security.rst (norbert-n)
a94d9f2 
This PR was submitted for the 2.3 branch but it was merged into the 2.7 branch instead (closes #6637).

Discussion
----------

Update security.rst

missing use statement

Commits
-------

5ac5d60 Update security.rst
@xabbuh
Copy link
Member

xabbuh commented Jun 11, 2016

Thank you @norbert-n. I have merged your fix into the 2.7 branch (that's why it's shown as closed instead of merged) as the 2.3 branch is no longer maintained.

@xabbuh xabbuh closed this Jun 11, 2016
@norbert-n norbert-n deleted the patch-1 branch June 11, 2016 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@norbert-n @OskarStark @javiereguiluz @wouterj @xabbuh