sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

make symfony/security-acl optional dependency #7303

Open Tobion opened 3 years ago

Tobion commented 3 years ago

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.

VincentLanglet commented 3 years ago

Indeed, we got an issue because someone wasn't using the acl-bundle https://github.com/sonata-project/SonataAdminBundle/issues/7086. So this can be related to https://github.com/sonata-project/SonataAdminBundle/issues/7097.

The Security\Acl namespace is used in

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[bot] commented 2 years ago

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 commented 1 year ago

I created a discussion about it on symfony to get some help https://github.com/symfony/symfony/discussions/48257

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

jordisala1991 commented 1 year ago

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 commented 1 year ago

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

jordisala1991 commented 1 year ago

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

Not an easy task tho.