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.

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

[Cookbook][Security] proofread comments in voter article #5598

Merged
merged 1 commit into from
Sep 5, 2015

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Aug 4, 2015

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

// It always will be, unless there is some misconfiguration of the
// security system.
// double-check that the User object is the expected entity
// it always will be unless there is some misconfiguration of the
Copy link
Contributor

Choose a reason for hiding this comment

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

the whole sentence sounds weird IMO

Copy link
Member

Choose a reason for hiding this comment

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

I agree. What about removing this part? // it always will be unless there is some misconfiguration of the security system

Copy link
Contributor

Choose a reason for hiding this comment

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

i would say that the first part is enough: // double-check that the User object is the expected entity.

EDIT: ah this is what you meant, right?

Copy link
Member

Choose a reason for hiding this comment

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

I believe @weaverryan liked to have the second sentence, indicating that this is a very rare exception.

(to be honest, I like having it as well...)

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe like this:

    // double-check that the User object is the expected entity.
    // this is a very rare case and only occurs, if you missconfigured your
    // security system.

case can be exchanged by exception

or

    // double-check that the User object is the expected entity.
    // this is a very rare case and only occurs, if your
    // security system is not well configured.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about:

double-check that the User object is the expected entity (this only happens when you did not configure the security system properly)

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me

Copy link
Member Author

Choose a reason for hiding this comment

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

updated here

@OskarStark
Copy link
Contributor

👍

@wouterj
Copy link
Member

wouterj commented Aug 20, 2015

👍

@xabbuh xabbuh merged commit 4d1de98 into symfony:2.3 Sep 5, 2015
xabbuh added a commit that referenced this pull request Sep 5, 2015
…(xabbuh)

This PR was merged into the 2.3 branch.

Discussion
----------

[Cookbook][Security] proofread comments in voter article

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets |

Commits
-------

4d1de98 proofread comments in voter article
@xabbuh xabbuh deleted the voter-comments branch September 5, 2015 10:37
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.

4 participants