Skip to content

Voter update#5908

Merged
weaverryan merged 4 commits into2.8from
voter-update
Nov 30, 2015
Merged

Voter update#5908
weaverryan merged 4 commits into2.8from
voter-update

Conversation

@weaverryan
Copy link
Copy Markdown
Contributor

Q A
Doc fix? no
New docs? yes
Applies to 2.8+
Fixed tickets #5728

@weaverryan
Copy link
Copy Markdown
Contributor Author

Here's the code behind this post: https://github.com/weaverryan/docs-voters/tree/finished

@wouterj
Copy link
Copy Markdown
Member

wouterj commented Nov 28, 2015

👍

@wouterj wouterj added this to the 2.8 milestone Nov 28, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why changing the namespace to use AppBundle\Security instead of AppBundle\Security\Authorization\Voter ?

If the application grows, it could have many things in the Security namespace. I think the cookbooks are good indications for a new developper to know where to place common things and promote some standard directory structures.

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.

I agree partially with @ogizanagi. Why not using AppBundle\Security\Voter as the namespace?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AppBundle\Security\Voter is fine to me too. 👍
I only have for habit to replicate the subnamespace of the component used for such things.

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.

As the best practice is to not clutter the AppBundle\Form with different sub-namespaces I'd say let's be consistent with that decision and do not introduce sub-namespaces here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with @xabbuh. There is no perfect answer, but I do hate telling people to create one directory, then another directory inside that directory before doing something.

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.

we should avoid using "we" here

@weaverryan
Copy link
Copy Markdown
Contributor Author

Thanks for the awesome review guys!

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