Ensure classes using the #[Entity] attribute are not declared as service

[GromNaN]

In order to enable discovery of entity classes by the Symfony DIC, we need to exclude them from being registered as actual services.

• We can remove the exclusion of the src/Entity/ directory from the symfony/framework-bundle recipe.
• Entity classes using the #[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#6943
• This is not using the new upcoming helpers for compatiblity with Symfony 6.4: [DependencyInjection] Add Definition::addExcludedTag() and ContainerBuilder::findExcludedServiceIds() for auto-discovering value-objects symfony/symfony#59704
• The same was done in DoctrineMongoDBODMBundle: Exclude classes with #[Document] attribute from container DoctrineMongoDBBundle#876

[stof]

this should also exclude MappedSuperclass and Embeddable

[GromNaN]

It's being done in the FrameworkBundle symfony/symfony#59987, but I think that's still valuable to add this in DoctrineBundle as that ease maintenance (if new attributes are added). At some point in the future, they can be removed from the FrameworkBundle: when all the versions of the DoctrineBundle that are supported by FrameworkBundle will have this exclusion.

[ostrolucky]

Can you check errors here https://github.com/doctrine/DoctrineBundle/actions/runs/13908353031/job/38916534370 ? They seem related

[GromNaN]

@ostrolucky see symfony/symfony#60001

[janklan]

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 ../src/Entity path was removed from the default exclusion list in the services.yaml file) and now they're not.

Wouldn't that be considered a BC break?

[GromNaN]

I assume you are using findTaggedServiceIds.

By adding the container.excluded tag, this definitions are no longer returned. You need to loop over all the service definitions as done for the #[JsonStreamable] attribute with json_streamer.streamable tag: https://github.com/symfony/symfony/blob/f047c9a3cd81745c16833f98001fe1c8d9e5f9e1/src/Symfony/Component/JsonStreamer/DependencyInjection/StreamablePass.php#L33

[ostrolucky]

Wouldn't that be considered a BC break?

Not the kind where we guarantee a BC for. Fixes break applications too, if you rely on buggy behaviour.

[janklan]

Wouldn't that be considered a BC break?

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.

[ostrolucky]

I don't think we should just call a long-standing practice a "buggy behaviour" and change it without any warning.

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.

Doing so undermines perceived reliability of the BC promise

Service definitions are not covered by Symfony BC promise. And DoctrineBundle does not officially follow Symfony BC promise anyways.

it should be marked deprecated and then phased out like any other obsolete behaviour.

I don't see any meaningful way to deprecate this without disrupting large portion of the ecosystem, do you?

[janklan]

Long standing practice is to have Entities excluded from service container

I agree with that. Another part of the long-standing practice was that you could change it, and now you can't.

smells like an oversight in your application

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.

Service definitions are not covered by Symfony BC promise

I didn't know that

And DoctrineBundle does not officially follow Symfony BC promise anyways.

Fair enough.

I don't see any meaningful way to deprecate this without disrupting large portion of the ecosystem, do you?

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".