-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[DoctrineBridge] Add argument to EntityValueResolver
to set type aliases
#54545
Conversation
49bdc51
to
5372f72
Compare
There was a problem hiding this 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)? 🙏
src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php
Outdated
Show resolved
Hide resolved
@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. |
5372f72
to
a04cfae
Compare
src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
a04cfae
to
e839a1f
Compare
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.
e839a1f
to
3aa64a3
Compare
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 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) |
EntityValueResolver
to set type aliases
I still think this makes sense. The diff is small enough and the linked issue explains the benefits quite well.
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. |
@nicolas-grekas I agree! In my opinion @stof Do you have any ideas or preferences on how to make entity interfaces in Doctrine and DoctrineBridge easier to work with? |
Friendly ping @symfony/mergers |
Thank you @NanoSector. |
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.