Skip to content

[Security] Split the component into 3 sub-components Core, ACL, HTTP #9047

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

Closed
wants to merge 1 commit into from

Conversation

webmozart
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

The rationale behind this PR is to be able to use any of the sub components without requiring all the dependencies of the other sub components. Specifically, I'd like to use the core utils for an improved CSRF protection mechanism (#6554).

TODO:

  • Decide what to do with the resources
  • Improve the text of the README files in the sub-components (@schmittjoh could you do this?)

@@ -0,0 +1,19 @@
Copyright (c) 2004-2013 Fabien Potencier
Copy link
Contributor

Choose a reason for hiding this comment

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

put the LICENSE under a meta folder according to best practices

Copy link
Member

Choose a reason for hiding this comment

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

Nope, that's only for bundles.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh ok sorry did not know, pardon me sil vou plait 👶

@fabpot
Copy link
Member

fabpot commented Sep 16, 2013

I'm definitely in favor of this split... if we can do it without BC breaks... so the old class names must still exist.

By the way, I think it would make #8848 obsolete.

You can run the unit tests with the following command:

$ cd path/to/Symfony/Component/Security/Http/
$ composer.phar install --dev
Copy link
Contributor

Choose a reason for hiding this comment

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

same here --dev

@webmozart
Copy link
Contributor Author

@fabpot The only class names that were changed so far are the tests, where the namespaces changed from Symfony\Component\Security\Tests\{Core|Acl|Http} to Symfony\Component\Security\{Core|Acl|Http}\Tests. I don't think the tests are part of our public API though.

I'm not quite sure what to do about the Resources directory. We could just leave it where it is, meaning you need to install symfony/security if you want to use the resources. Moving them would be a BC break.

@stof
Copy link
Member

stof commented Sep 16, 2013

@fabpot There is no BC breaks. Only test classes are renamed in this PR. All other classes are still the same.

for the resources, I think we should find a way to have them in one of the components. Having to require the symfony/security component to get them kind of defeats the possibility to avoid installing the ACL system.

@webmozart
Copy link
Contributor Author

@stof Well one possibility would be to move them to wherever they belong (Core or HTTP?), and to add a Composer post-install hook that copies them to the main directory in symfony/security.

@fabpot
Copy link
Member

fabpot commented Sep 16, 2013

I suppose that the goal is to move the classes themselves as well, right?

@stof
Copy link
Member

stof commented Sep 16, 2013

@bschussek a post-install hook is not a good BC layer as it has to be added by the user in the root composer.json

@stof
Copy link
Member

stof commented Sep 16, 2013

@fabpot The Security component is already divided in 3 distinct namespaces. This PR simply creates the subtree splits of these 3 folders.

@fabpot
Copy link
Member

fabpot commented Sep 16, 2013

... I need to sleep more ;)

@fabpot
Copy link
Member

fabpot commented Sep 17, 2013

For the Resources directory, I would copy to Core (as Http depends on Core), deprecate the old location, and remove it in 3.0. In the meantime, we must ensure that both Resources directory are kept in sync.

@fabpot
Copy link
Member

fabpot commented Sep 18, 2013

Closing in favor of #9064

@fabpot fabpot closed this Sep 18, 2013
fabpot added a commit that referenced this pull request Sep 18, 2013
This PR was merged into the master branch.

Discussion
----------

[Security] Split the component into 3 sub-components Core, ACL, HTTP

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9047, #8848
| License       | MIT
| Doc PR        | -

The rationale behind this PR is to be able to use any of the sub components without requiring all the dependencies of the other sub components. Specifically, I'd like to use the core utils for an improved CSRF protection mechanism (#6554).

Commits
-------

14e9f46 [Security] removed unneeded hard dependencies in Core
5dbec8a [Security] fixed README files
62bda79 [Security] copied the Resources/ directory to Core/Resources/
7826781 [Security] Split the component into 3 sub-components Core, ACL, HTTP
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