Skip to content

add support for doctrine/persistence 4#123

Open
dmaicher wants to merge 1 commit into
symfony:mainfrom
dmaicher:doctrine_persistence_4
Open

add support for doctrine/persistence 4#123
dmaicher wants to merge 1 commit into
symfony:mainfrom
dmaicher:doctrine_persistence_4

Conversation

@dmaicher

@dmaicher dmaicher commented Feb 3, 2025

Copy link
Copy Markdown

Adds compatibility with doctrine/persistence 4

.PHP Fatal error:  Declaration of Symfony\Component\Security\Acl\Domain\Acl::addPropertyChangedListener(Doctrine\Persistence\PropertyChangedListener $listener) must be compatible with Doctrine\Persistence\NotifyPropertyChanged::addPropertyChangedListener(Doctrine\Persistence\PropertyChangedListener $listener): void in /var/www/security-acl/Domain/Acl.php on line 70
......PHP Fatal error:  Declaration of Symfony\Component\Security\Acl\Dbal\MutableAclProvider::propertyChanged($sender, $propertyName, $oldValue, $newValue) must be compatible with Doctrine\Persistence\PropertyChangedListener::propertyChanged(object $sender, string $propertyName, mixed $oldValue, mixed $newValue): void in /var/www/security-acl/Dbal/MutableAclProvider.php on line 169

Q: technically this is a BC break for users who extended those classes + methods since the classes are not @final 🤔 Any other suggestion?

@wouterj

wouterj commented Feb 14, 2025

Copy link
Copy Markdown
Member

Hi! Thanks for proposing this!

This is indeed a BC break, so this would need to be a new major version. The @return annotations will trigger deprecations in Symfony applications if the overridden method does not have a native return type, so the upgrade path is already smooth in this situation.

We might want to do more things in a new major version. I'll create an issue about it to discuss with the rest of the core team. Let's wait for a decision before merging this PR.

@lazb

lazb commented Feb 14, 2025

Copy link
Copy Markdown

@wouterj sorry for jumping in here, but I just want to ask if it would make sense to check other open issues, if any would be a candidate to be merged for v4?

I can only add that in one of my projects we are using this PR as a composer patch for years now: #23

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.

3 participants