Skip to content

[Security] Removed security-acl from the core #15013

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Security] Removed security-acl from the core #15013

wants to merge 2 commits into from

Conversation

linaori
Copy link
Contributor

@linaori linaori commented Jun 17, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets part of #14718
License MIT
Doc PR ~

The Security\Acl is removed from the core and is loaded from its own repository. All tests were passing and this is fully backwards compatible. I have removed all but the Test files in the first step and added the dependency to verify the Test were still working with the package dependency. The second step was to remove the remaining test files and tests are still running for both the Bundle and the Framework. Once the Read-Only repository is a full standalone repository, this PR can be merged.

Once this PR is merged, I can start working on splitting the SecurityBundle and extracting the ACL part to the AclBundle.

/cc @fabpot

@stof
Copy link
Member

stof commented Jun 17, 2015

symfony/security should not replace symfony/security-acl anymore either

stof
stof reviewed Jun 17, 2015
View reviewed changes
src/Symfony/Bundle/SecurityBundle/composer.json
@@ -18,7 +18,8 @@
"require": {
"php": ">=5.3.9",
"symfony/security": "~2.8|~3.0.0",
"symfony/http-kernel": "~2.2|~3.0.0"
"symfony/http-kernel": "~2.2|~3.0.0",
"symfony/security-acl": "~2.8|~3.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

should it really be a required dependency ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To preserve backwards compatibility as it's always available atm. Once the Acl Bundle is created, I can move this dependency there and put the bundle as dependency in the SecurityBundle. In 3.0 the BC layer (regarding config) + dependency can be removed.

@linaori
Copy link
Contributor Author

linaori commented Jun 17, 2015

@stof ah yes, I will update those accordingly

@stof
Copy link
Member

stof commented Jun 17, 2015

btw, the not-removed replacement is the reason why tests are failing

@linaori
Copy link
Contributor Author

linaori commented Jun 17, 2015

Lets see what happens after a push. For some reason my 2.8 keeps complaining about objects not being equal in form tests;

73) Symfony\Component\Validator\Tests\Constraints\RangeValidatorTest::testInvalidDatesCombinedMin with data set #3 (DateTimeImmutable Object (...), 'Mar 9, 2014, 12:00 AM')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 Symfony\Component\Validator\ConstraintViolation Object (
     'message' => null
     'messageTemplate' => 'myMinMessage'
     'parameters' => Array (
-        '{{ value }}' => 'Mar 9, 2014, 12:00 AM'
-        '{{ limit }}' => 'Mar 10, 2014, 12:00 AM'
+        '{{ value }}' => 'Mar 9, 2014 12:00 AM'
+        '{{ limit }}' => 'Mar 10, 2014 12:00 AM'
     )
     'plural' => null
     'root' => 'root'
     'propertyPath' => 'property.path'
     'invalidValue' => 'InvalidValue'
     'constraint' => Symfony\Component\Validator\Constraints\NotNull Object (...)
     'code' => 3
     'cause' => null
 )

@fabpot
Copy link
Member

fabpot commented Aug 1, 2015

Thank you @iltar.

@fabpot fabpot closed this in bffca95 Aug 1, 2015
@fabpot
Copy link
Member

fabpot commented Aug 1, 2015

As of today, the ACL subtree split is not managed by the splitter anymore as of branch 2.8. For older branches, it is still active, even if there are close to no activity on the component. I've also activated the ticketing system on the repo.

@fabpot
Copy link
Member

fabpot commented Aug 1, 2015

@iltar The next step would be to create the AclBundle and remove everything related to Acl in core (you had some good ideas in the linked issue).

@linaori
Copy link
Contributor Author

linaori commented Aug 1, 2015

Thanks! I will get on it.

@linaori linaori mentioned this pull request Aug 12, 2015
Closed
@jakzal jakzal mentioned this pull request Sep 6, 2015
Closed
@fabpot fabpot mentioned this pull request Nov 16, 2015
Merged
@jakzal jakzal mentioned this pull request Jan 15, 2016
Closed
@linaori linaori deleted the feature/security-acl-split branch April 15, 2016 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@linaori @stof @fabpot @aitboudad