search

KnpLabs / DoctrineBehaviors

Doctrine2 behavior traits that help handling Blameable, Loggable, Sluggable, SoftDeletable, Uuidable, Timestampable, Translatable, Tree behavior
http://knplabs.com
MIT License
911 stars 287 forks source link

Registering doctrine subscribers automatically is an issue #625

Closed Nek- closed 2 years ago

Nek- commented 3 years ago

Explanation

This package contains a bundle that automatically registers doctrine subscribers as subscribers in the container builder. It has been added here to "fix" the fact that the doctrine bundle is not doing it by itself. The point is that doctrine bundle is now doing it. By using a subclass of the subscriber.

This instruction is now conflicting with the doctrine bundle if you use it well. (all this story leads to undefined behavior, the only thing I was able to understand is that it's basically broken)

https://github.com/KnpLabs/DoctrineBehaviors/blob/6b0a0be53edd5096afe8a700d81b7bec8e1ace5e/src/Bundle/DependencyInjection/DoctrineBehaviorsExtension.php#L29

Available solutions

  1. We could just remove it and make register explicitly subscribers. I know it's 8 service declarations to write by hand but I believe it's worth it.
  2. We could remove this line and make the subscribers implement the correct interface.

Implications

Either way, it will have a little backward compatibility break:

  1. For the first case, if people are not following good practices and implements the same interface as we do in this bundle. Especially considering that the documentation of Symfony is not up to date! (see related issue)
  2. People will obviously have first case problems in the second case, but in addition if the doctrine bundle version is older than 2.1.0 this library will not work. This means we need to update the minimum version for doctrine bundle in this case.
TomasVotruba commented 2 years ago

Hi, thanks for reporting this issue. I've just got came across this report.

I don't have much time to maintain this repository. I've picked one of your suggestions and implemented it. For next time, if you can submit PR and make CI pass, that would be huge help for me :heart: . I could review and merge it in matter of hours :+1:


It is resolved here: https://github.com/KnpLabs/DoctrineBehaviors/pull/649/files

Nek- commented 2 years ago

This is awesome thank you very much @TomasVotruba . I should have suggested making the PR, I prefer asking first to avoid useless work, but I agree I was not clear about the fact that I can do it.

TomasVotruba commented 2 years ago

No problem, I usually do the same :smile: Seing your comment and suggestions, I'm confident you can propose working valid solution and consider the consequences propperly. So next time feel free to go for it :sunglasses: :+1: