Skip to content

[DoctrineBridge] Add argument to EntityValueResolver to set type aliases #54545

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

Conversation

NanoSector
Copy link
Contributor

@NanoSector NanoSector commented Apr 10, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #51765
License MIT

This allows for fixing #51765; with a consequential Doctrine bundle update, the resolve_target_entities configuration can be injected similarly to ResolveTargetEntityListener in the Doctrine codebase.

Alternatively the config and ValueResolver can be injected using a compiler pass in the Symfony core code, however the value resolver seems to be configured in the Doctrine bundle already.

@NanoSector NanoSector force-pushed the feature/51765-entityvalueresolver-alias-aware branch from 49bdc51 to 5372f72 Compare April 10, 2024 19:22
@nicolas-grekas nicolas-grekas added this to the 7.1 milestone Apr 11, 2024
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Could you please open the corresponding PR on doctrine-bundle so that we can have the full picture (populating $typeAliases)? 🙏

@NanoSector
Copy link
Contributor Author

@nicolas-grekas Thanks for your review, I’ll try to open the corresponding Doctrine bundle PR later today or this week; it’s mostly extension configuration and injecting the right config into the added array here. Depending on the feedback here I can expand either PR.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM, I just have minor comments. Please rebase also.

@NanoSector NanoSector force-pushed the feature/51765-entityvalueresolver-alias-aware branch from a04cfae to e839a1f Compare August 12, 2024 20:42
This allows for fixing symfony#51765; with a consequential Doctrine bundle update, the resolve_target_entities configuration can be injected similarly to ResolveTargetEntityListener in the Doctrine codebase.

Alternatively the config and ValueResolver can be injected using a compiler pass in the Symfony core code, however the value resolver seems to be configured in the Doctrine bundle already.
@nicolas-grekas nicolas-grekas force-pushed the feature/51765-entityvalueresolver-alias-aware branch from e839a1f to 3aa64a3 Compare August 19, 2024 09:38
@stof
Copy link
Member

stof commented Sep 20, 2024

I don't think repurposing the target_entities configuration is a good thing. In the ORM, target entities work only in the mapping configuration for the targetEntity setting of relations. It does not work anywhere else. You cannot do $em->find(MyInterface::class, $id) and expect the interface to be resolved.
This PR is about emulating such find usage though.

Making the EntityValueResolver diverge from the behavior of the ORM while reusing the ORM configuration setting for that is likely to become a maintenance nightmare (as it ties this feature to the ORM feature, which could break it if the ORM changes things in its own feature)

@OskarStark OskarStark changed the title [DoctrineBridge] Add argument to EntityValueResolver to set type aliases [DoctrineBridge] Add argument to EntityValueResolver to set type aliases Sep 20, 2024
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 18, 2024

I still think this makes sense. The diff is small enough and the linked issue explains the benefits quite well.
Attributes are about allowing more descriptive code, improving the DX, and here, I can see where the DX is improved.

You cannot do $em->find(MyInterface::class, $id)

Well, maybe that could be a feature request to the ORM?

As far as this PR is concerned, this is still very much decoupled from the ORM, so I don't think there is any risk (not to say the ORM won't remove a feature without extensive warnings / deprecations so again, no risk here).

Still 👍 to me.

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@aleho
Copy link
Contributor

aleho commented Jan 14, 2025

@nicolas-grekas I agree! In my opinion resolve_target_entities and support for interfaces is not complete in Doctrine. We can define interfaces in relations, but we cannot use them in perfectly valid scenarios like MapEntity or AsEntityListener.

@stof Do you have any ideas or preferences on how to make entity interfaces in Doctrine and DoctrineBridge easier to work with?

@nicolas-grekas
Copy link
Member

Friendly ping @symfony/mergers

@nicolas-grekas
Copy link
Member

Thank you @NanoSector.

@nicolas-grekas nicolas-grekas merged commit 146c128 into symfony:7.3 Mar 22, 2025
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants