Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[DoctrineBridge] Add argument toEntityValueResolver 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

NanoSector
Copy link
Contributor

@NanoSectorNanoSector commentedApr 10, 2024
edited by nicolas-grekas
Loading

QA
Branch?7.2
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix#51765
LicenseMIT

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.

ndench reacted with thumbs up emoji
@NanoSectorNanoSectorforce-pushed thefeature/51765-entityvalueresolver-alias-aware branch from49bdc51 to5372f72CompareApril 10, 2024 19:22
@nicolas-grekasnicolas-grekas added this to the7.1 milestoneApr 11, 2024
Copy link
Member

@nicolas-grekasnicolas-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
ContributorAuthor

@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-grekasnicolas-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.

@NanoSectorNanoSectorforce-pushed thefeature/51765-entityvalueresolver-alias-aware branch froma04cfae toe839a1fCompareAugust 12, 2024 20:42
This allows for fixingsymfony#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-grekasnicolas-grekasforce-pushed thefeature/51765-entityvalueresolver-alias-aware branch frome839a1f to3aa64a3CompareAugust 19, 2024 09:38
@stof
Copy link
Member

I don't think repurposing the target_entities configuration is a good thing. In the ORM, target entities workonly in the mapping configuration for thetargetEntity setting of relations. It does not workanywhere else. Youcannot do$em->find(MyInterface::class, $id) and expect the interface to be resolved.
This PR is about emulating suchfind 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)

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

nicolas-grekas commentedOct 18, 2024
edited
Loading

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.

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

@nicolas-grekas I agree! In my opinionresolve_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 likeMapEntity orAsEntityListener.

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

NanoSector reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commit146c128 intosymfony:7.3Mar 22, 2025
8 of 10 checks passed
@fabpotfabpot mentioned this pull requestMay 2, 2025
nicolas-grekas added a commit that referenced this pull requestMay 9, 2025
This PR was merged into the 7.3 branch.Discussion----------[DoctrineBridge] fix changelog| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        || License       | MIT#54545 was merged into the `7.3` branchCommits-------03e0830 fix changelog
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

[DoctrineBridge] #[MapEntity] does not resolve an entity from its interface, aliased using resolve_target_entities
6 participants
@NanoSector@stof@nicolas-grekas@aleho@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp