Skip to content

Collection of fixes and improvements #3595

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 2 commits into from
Mar 7, 2014
Merged

Collection of fixes and improvements #3595

merged 2 commits into from
Mar 7, 2014

Conversation

bicpi
Copy link
Contributor

@bicpi bicpi commented Feb 20, 2014

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

This PR fixes all kind of typos, formatting and code issues.
It also adds some missing code blocks and notes or improvements.

Thanks for your review and please let me know if you see something wrong.

@bicpi
Copy link
Contributor Author

bicpi commented Feb 21, 2014

If I've mixed too much topics I may split this PR.

@@ -34,7 +34,7 @@ Basic Example: HTTP Authentication
The Security component can be configured via your application configuration.
In fact, most standard security setups are just a matter of using the right
configuration. The following configuration tells Symfony to secure any URL
matching ``/admin/*`` and to ask the user for credentials using basic HTTP
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to instead fix the code example

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 also thought about this option which would mean to change the access_control rule to ^/admin/.

But maybe users are not aware that securing with ^/admin/ excludes a possible admin area entry point /admin?

Copy link
Member

Choose a reason for hiding this comment

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

good point! Let's keep it like this.

I don't like /admin* btw, can't we change it to show that only once and say "starts with /admin" in other occurences?

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 totally agree that /admin* looks ugly.

What about adding a 2nd access rule in the code for ^/admin$ with a code comment that this is necessary to also secure the /admin path itself? I think this is used somewhere else to secure ^/login$.

Copy link
Member

Choose a reason for hiding this comment

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

+1

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2014

👍

@weaverryan
Copy link
Member

WOW, I can't believe how many tweaks you found - that's incredible! Thanks a ton for this. Also, I like what you guys decided on for the access control part - very clear. Cheers!

weaverryan added a commit that referenced this pull request Mar 7, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

Collection of fixes and improvements

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

This PR fixes all kind of typos, formatting and code issues.
It also adds some missing code blocks and notes or improvements.

Thanks for your review and please let me know if you see something wrong.

Commits
-------

56e475c Fixes after review
8d4f444 Fixes of all kind
@weaverryan weaverryan merged commit 56e475c into symfony:2.3 Mar 7, 2014
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