Skip to content
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

make symfony/security-acl optional dependency #7303

Open
Tobion opened this issue Jul 4, 2021 · 6 comments
Open

make symfony/security-acl optional dependency #7303

Tobion opened this issue Jul 4, 2021 · 6 comments

Comments

@Tobion
Copy link

Tobion commented Jul 4, 2021

Using Symfony ACL is not recommended for most situations anymore. I also think sonata should not require symfony/security-acl component by default and instead it should be an optional dependency in case people really want acl functionality. This is somehow the case already as sontata would also require symfony/acl-bundle in practice which it does not require. So people need to require certain packages manually to add acl support.

Currently sonata/admin-bundle and doctrine-orm-admin-bundle require symfony/security-acl. Some classes implement interfaces from security-acl like AbstractAdmin implements Symfony\Component\Security\Acl\Model\DomainObjectInterface.
So I think it would be better if sonata runs without acl by default and people wanting acl functionality would just implement acl interfaces in their own classes.

@Tobion Tobion added the feature label Jul 4, 2021
@Tobion Tobion changed the title make symfony/security-acl optional dependecy make symfony/security-acl optional dependency Jul 4, 2021
@VincentLanglet
Copy link
Member

Indeed, we got an issue because someone wasn't using the acl-bundle #7086. So this can be related to #7097.

The Security\Acl namespace is used in

  • AbstractAdmin which implements DomainObjectInterface
  • GenerateObjectAclCommand
  • AdminPermissionMap
  • MaskBuilder
  • AclSecurityHandler (and interface)
  • SecurityExtension
  • AdminAclManipulator (and interface)
  • AdminObjectAclData
  • AdminObjectAclManipulator
  • ObjectAclManipulator (and interface)
    it also used in the config
set('sonata.admin.security.mask.builder.class', MaskBuilder::class)

In SonataDoctrineORM, it's just used in the ObjectAclManipulator.

What is the way to make this dependency optional ? Do you want to make the PR ?

I assume that the AbstractAdmin won't work if it implements a non-existing interface.

This might be a good idea to move the Acl related class from Util to an Acl folder too @sonata-project/contributors

@github-actions
Copy link

github-actions bot commented Jan 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@VincentLanglet
Copy link
Member

I created a discussion about it on symfony to get some help
symfony/symfony#48257

WDYT about this @jordisala1991 ?
How should we update our code to make acl optionnal ?

@jordisala1991
Copy link
Member

Yes, I think we should make it optional. And if we manage to split it in a separate bundle, even better, never used ACL and when I tried, never got to make it work.

@VincentLanglet
Copy link
Member

I dunno how we can split since, for example AbstractAdmin implements DomainObjectInterface...

@jordisala1991
Copy link
Member

Yep, we need to tackle like other things we split from the adminInterface, like breadcrumbs, templates...

Not an easy task tho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants