Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
weaverryan merged 2 commits intosymfony:2.3frombicpi:fix_collection
Mar 7, 2014
Merged

Collection of fixes and improvements#3595

weaverryan merged 2 commits intosymfony:2.3frombicpi:fix_collection
Mar 7, 2014

Conversation

@bicpi
Copy link
Contributor

QA
Doc fix?yes
New docs?no
Applies toall
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
ContributorAuthor

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

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
ContributorAuthor

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 theaccess_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
ContributorAuthor

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

👍

@weaverryan
Copy link
Member

WOW, I can't believe how many tweaks you found - that's incredible! Thanks aton 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 requestMar 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 review8d4f444 Fixes of all kind
@weaverryanweaverryan merged commit56e475c intosymfony:2.3Mar 7, 2014
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bicpi@xabbuh@weaverryan@wouterj

[8]ページ先頭

©2009-2025 Movatter.jp