Skip to content

review all Security code blocks#5486

Merged
wouterj merged 1 commit intosymfony:2.3from
xabbuh:security-code-blocks
Jul 21, 2015
Merged

review all Security code blocks#5486
wouterj merged 1 commit intosymfony:2.3from
xabbuh:security-code-blocks

Conversation

@xabbuh
Copy link
Copy Markdown
Member

@xabbuh xabbuh commented Jul 5, 2015

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets #4606 (comment)

As I promised @weaverryan I now found some time to review all the security-related code blocks. :)

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.

rule and access_control do have the same semantic meaning in XML (actually access_control is the pluralised form of rule). In fact, nesting access-control and rule was a mistake we made in the documentation and only worked if we had only one rule.

The resulting array after processing the configuration would have been an array with rule as the key. This only worked because the extension just loops the array and doesn't deal with its keys. Though obviously this will break as soon as you have more than one rule.

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.

Actually, I am not sure if we should really use the rule element at all. Would it be more clear if we simply used multiple access-control elements?

@xabbuh xabbuh force-pushed the security-code-blocks branch 7 times, most recently from 3b08fb1 to 49e589c Compare July 5, 2015 11:07
@javiereguiluz
Copy link
Copy Markdown
Member

@xabbuh first, thank you very much for this huge work! I have a question: do you still consider this PR as WIP? I'm asking because I've reviewed it and it looks ready to be merged.

@xabbuh
Copy link
Copy Markdown
Member Author

xabbuh commented Jul 7, 2015

Yeah, there five or six files from the cookbook section I didn't look at yet.

@xabbuh xabbuh force-pushed the security-code-blocks branch from 49e589c to 04fc6a6 Compare July 7, 2015 19:22
@xabbuh xabbuh changed the title [WIP] review all Security code blocks review all Security code blocks Jul 7, 2015
@xabbuh
Copy link
Copy Markdown
Member Author

xabbuh commented Jul 7, 2015

This is ready to be reviewed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the examples above, we usually define the pattern as ^/api instead of /api/.* Is there any reason for using this other format?

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 don't know the reason for this (maybe just a matter of taste of the initial author of that file). I changed it for consistency.

@javiereguiluz
Copy link
Copy Markdown
Member

@xabbuh I've reviewed this PR again and I love it. Thank you and congrats on this huge work.

@xabbuh xabbuh force-pushed the security-code-blocks branch from 04fc6a6 to 2159bda Compare July 8, 2015 06:57
@OskarStark
Copy link
Copy Markdown
Contributor

Thank you @xabbuh!

👍

@xabbuh xabbuh force-pushed the security-code-blocks branch from 2159bda to 34f7859 Compare July 16, 2015 20:27
@xabbuh xabbuh force-pushed the security-code-blocks branch from 34f7859 to 9099cf2 Compare July 16, 2015 20:29
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

while this works, we always use attributes for non-array nodes (making this <acl connection="default"/>

@wouterj wouterj merged commit 9099cf2 into symfony:2.3 Jul 21, 2015
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.

5 participants