Ensure classes using the #[Entity] attribute are not declared as service
#1868
Conversation
b1f7454 to
d5297d7
Compare
ac60cf1 to
c33b49d
Compare
c33b49d to
bbcce03
Compare
|
this should also exclude MappedSuperclass and Embeddable |
bbcce03 to
3a31649
Compare
|
It's being done in the |
|
Can you check errors here https://github.com/doctrine/DoctrineBundle/actions/runs/13908353031/job/38916534370 ? They seem related |
…nfiguration callbacks on the same class (GromNaN) This PR was squashed before being merged into the 7.3 branch. Discussion ---------- [DependencyInjection] Enable multiple attribute autoconfiguration callbacks on the same class | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | yes | New feature? | yes | Deprecations? | yes | Issues | Fix doctrine/DoctrineBundle#1868 (comment) | License | MIT Replace #60001 By having a list of callables for each attributes, we can enable merging definitions each having an autoconfiguration for the same attribute class. This is the case with the `#[Entity]` attribute in DoctrineBundle and FrameworkBundle. I have to deprecate `ContainerBuilder::getAutoconfiguredAttributes()` as its return type is `array<class-string, callable>`; so I added a new method `AttributeAutoconfigurationPass` that returns `array<class-string, callable[]>` in in order to use reflection on each callable in the compiler pass. Commits ------- e36fe60 [DependencyInjection] Enable multiple attribute autoconfiguration callbacks on the same class
|
Hello all. I did a minor version upgrade of DoctrineBundle, and my app broke because of this change. The entities could have been part of the DI container for ages (assuming the Wouldn't that be considered a BC break? |
|
I assume you are using By adding the |
Not the kind where we guarantee a BC for. Fixes break applications too, if you rely on buggy behaviour. |
Hmm. I don't think we should just call a long-standing practice a "buggy behaviour" and change it without any warning. Doing so undermines perceived reliability of the BC promise, I think. It's a behaviour, buggy or not. It is now considered obsolete. I think it should be marked deprecated and then phased out like any other obsolete behaviour. |
Long standing practice is to have Entities excluded from service container, I don't think you should ignore that either. That really smells like an oversight in your application.
Service definitions are not covered by Symfony BC promise. And DoctrineBundle does not officially follow Symfony BC promise anyways.
I don't see any meaningful way to deprecate this without disrupting large portion of the ecosystem, do you? |
I agree with that. Another part of the long-standing practice was that you could change it, and now you can't.
Including entities in the container might be a bad call if it was used to turn Entities into classes with things autowired into them. I didn't include them for that purpose and explained my reasons here.
I didn't know that
Fair enough.
TL;DR: Can't you at least throw a warning while compiling the container that the entities are now forcefully excluded? Full reply: I found this because our CI/CD pipeline started failing: one of the fixtures generators was inserting a null value into a not-null column. The value is auto-generated by an insert trigger. For some reason, the trigger was not firing. Looking closer, the trigger wasn't there at all. After some stumbling around, I found the custom queries were not executed during the migration process, and the reason for that was that the migration listener responsible for finding the entities and executing their custom queries was no longer receiving their list from the container. Things were still working fine on previously deployed systems because all of the custom stuff was already set up. I think that's a pretty stealthy bug. If there was any sort of warning, I'd probably say "Oh, yeah, no worries". |
In order to enable discovery of entity classes by the Symfony DIC, we need to exclude them from being registered as actual services.
src/Entity/directory from thesymfony/framework-bundlerecipe.#[ApiResource]attribute can be discovered using DI instead of using a custom-made mechanism of directory scanning and cache: feat(symfony): Autoconfigure classes using#[ApiResource]attribute api-platform/core#6943Definition::addExcludedTag()andContainerBuilder::findExcludedServiceIds()for auto-discovering value-objects symfony/symfony#59704#[Document]attribute from container DoctrineMongoDBBundle#876