Skip to content

Security cookbook pass #1825

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 8 commits into from
Oct 23, 2012
Merged

Conversation

greg0ire
Copy link
Contributor

A couple of corrections, please read carefully, I'm not dead sure about all of them.

@@ -5,9 +5,9 @@ How to force HTTPS or HTTP for Different URLs
=============================================

You can force areas of your site to use the ``HTTPS`` protocol in the security
config. This is done through the ``access_control`` rules using the ``requires_channel``
config. This is done through the ``access_control`` rules, using the ``requires_channel``
Copy link
Contributor

Choose a reason for hiding this comment

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

This comma is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what was meant here: who is using the requires_channel? The developer who writes the code (which made me add this comma), or is it the access_control rules? But maybe it does not even matter. What do you think?

If you really want me to remove this comma, how should I do this? Should I add a reverting commit to this pull request? Or make another pull request?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure, but I think that doing an stop there is odd. That's why I suggest that.
If you revert you'll lose the other change, so simple do a different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure, but I think that doing an stop there is odd.

It feels less odd to me without the comma, so if you're not sure, I propose we let @weaverryan decide, I believe he's a native english speaker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine! :)

Copy link
Member

Choose a reason for hiding this comment

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

It is actually better without the comma - it's just one straight thought, similar to I got to the hotel using a taxi.

But, the vast majority of what you had here is perfect.

Cheers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nomack84 : looks like you were right :) !
@weaverryan : I trust I don't have anything do to, it looks like you managed apply the PR without this commit...

weaverryan added a commit that referenced this pull request Oct 23, 2012
@weaverryan weaverryan merged commit 7d6c013 into symfony:2.0 Oct 23, 2012
weaverryan added a commit that referenced this pull request Oct 23, 2012
@weaverryan
Copy link
Member

Hi guys!

Lot's of nice changes here. I've merged them in and made a few small tweaks.

Thanks - these small PR's a so great to keeping the docs high quality!

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.

4 participants