-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Conversation
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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$
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
👍 |
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! |
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
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.