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

Allow Symfony 7 #1669

Open
wants to merge 8 commits into
base: 5.x
Choose a base branch
from
Open

Allow Symfony 7 #1669

wants to merge 8 commits into from

Conversation

jordisala1991
Copy link
Member

Subject

I am targeting this branch, because {reason}.

Closes #{put_issue_number_here}.

Changelog

### Added
- Some `Class::newMethod()` to do great stuff

### Changed

### Deprecated

### Removed

### Fixed

### Security


final class UserAclVoter extends AclVoter
final class UserAclVoter implements VoterInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure why we needed to extend the acl class if this is not using anything from it, do you know @VincentLanglet ?

The acl package is not really compatible with sf 7.0, the aclvoter misses some typehints

Doing this change is bc break. What should we do?

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea why it was done like this...

But the security-acl was supposed to be SF-7 compatible https://github.com/symfony/security-acl/blob/main/composer.json ; is there somethng to report to symfony ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this interface is not ready for SF 7: https://github.com/symfony/security-acl/blob/main/Voter/AclVoter.php, there are no typehints.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed we need symfony/security-acl#112

Copy link
Member Author

@jordisala1991 jordisala1991 Dec 19, 2023

Choose a reason for hiding this comment

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

The security-acl package is only used here, for an extends that does not really make sense. Unless I am missing something, the only BC break would be if someone tries to instantiate this class (since it is final) or redeclares the dependency injection... but this class is not expected to be construct by anyone else, since it is hook on the security system using the voter tag.

We could remove dep here and implement normal voter interface. The BC break is not really big

If someone uses security-acl, they need to add the bundle, and the component, and configure them on the security yaml.

Copy link
Member

Choose a reason for hiding this comment

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

There is some ACL-related doc in this bundle like https://github.com/sonata-project/SonataUserBundle/blob/5.x/docs/reference/installation.rst#acl-configuration

I dunno if we extends the AclVoter to decorate it and replacing it in the service injection. :(
But I never used this bundle...

If we don't need ACL here, let's remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

security.acl.voter.user_permissions

It could be replacing the service indeed, but AFAIK the service is only declared on the bundle: https://github.com/symfony/acl-bundle/blob/main/src/Resources/config/acl.xml#L25C9-L25C114

So it is not really replacing it, but declaring another one, the name is different.

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to remove the extends then

Copy link
Member

Choose a reason for hiding this comment

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

I've created a PR on the securty-acl bundle that is only waiting to be merged
symfony/security-acl#116

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.

None yet

3 participants