Skip to content
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

Rework association mapping hierarchy #10681

Merged
merged 2 commits into from May 7, 2023

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented May 5, 2023

Please review #10682 before this one.

  • Each type is now either final, abstract or an interface. Inverse mappings are no longer represented by intermediary types, which means you no longer have this weird inheritance between owning and inverse.
  • The mappedBy attribute is no longer nullable and moved down the hierarchy, because it makes sense only for inverse side mappings.
  • The inversedBy attribute is still nullable and also moved down the hierarchy, because it makes sense only for owning side mappings.
  • Code common to ManyToOneAssociationMapping and OneToOneOwningSideMapping is de-duplicated and moved up the hierarchy
  • Code inside ToManyInverseSideMapping and ToManyOwningSideMapping comes from a trait (:nauseated_face:) to avoid duplication.

Before

Classes

classDiagram
    AssociationMapping <|-- ToManyAssociationMapping
    ToManyAssociationMapping <|-- ManyToManyAssociationMapping
    ManyToManyAssociationMapping <|-- ManyToManyOwningSideMapping
    AssociationMapping <|-- ToOneAssociationMapping
    ToOneAssociationMapping <|-- ManyToOneAssociationMapping
    ToManyAssociationMapping <|-- OneToManyAssociationMapping
    ToOneAssociationMapping <|-- OneToOneAssociationMapping
    OneToOneAssociationMapping <|-- OneToOneOwningSideMapping

Interfaces

classDiagram
    AssociationOwningSideMapping <|.. OneToOneOwningSideMapping
    AssociationOwningSideMapping <|.. ManyToManyOwningSideMapping
    AssociationOwningSideMapping <|.. ManyToOneAssociationMapping

After

Classes

classDiagram
    AssociationMapping <|-- InverseSideMapping
    AssociationMapping <|-- OwningSideMapping
    InverseSideMapping <|-- ToManyInverseSideMapping
    OwningSideMapping <|-- ToManyOwningSideMapping
    ToManyInverseSideMapping <|-- ManyToManyInverseSideMapping
    ToManyOwningSideMapping <|-- ManyToManyOwningSideMapping
    OwningSideMapping <|-- ToOneOwningSideMapping
    ToOneOwningSideMapping <|-- ManyToOneOwningSideMapping
    ToManyInverseSideMapping <|-- OneToManyAssociationMapping
    InverseSideMapping <|-- ToOneInverseSideMapping
    ToOneInverseSideMapping <|-- OneToOneInverseSideMapping
    ToOneOwningSideMapping <|-- OneToOneOwningSideMapping

Interfaces

classDiagram
    ToManyAssociationMapping <|.. ManyToManyAssociationMapping
    ManyToManyAssociationMapping <|.. ManyToManyInverseSideMapping
    ManyToManyAssociationMapping <|.. ManyToManyOwningSideMapping
    ToOneAssociationMapping <|.. OneToOneAssociationMapping
    OneToOneAssociationMapping <|.. OneToOneInverseSideMapping
    OneToOneAssociationMapping <|.. OneToOneOwningSideMapping

ToManyAssociationMappingImplementation Trait

classDiagram
    AssociationMapping <|-- InverseSideMapping
    AssociationMapping <|-- OwningSideMapping
    InverseSideMapping <|-- ToManyInverseSideMapping
    OwningSideMapping <|-- ToManyOwningSideMapping
    ToManyAssociationMappingImplementation <|-- ToManyInverseSideMapping
    ToManyAssociationMappingImplementation <|-- ToManyOwningSideMapping

Instead of ensuring every mapping array has a mappedBy and an inversedBy
field, let us do the opposite, and remove them when they are null.
Likewise if there is a joinColumns field, it is useless if null or
empty.
@greg0ire greg0ire force-pushed the rework-class-hierarchy branch 3 times, most recently from e901331 to e650ed1 Compare May 5, 2023 21:23
@greg0ire greg0ire marked this pull request as ready for review May 7, 2023 16:54
@greg0ire
Copy link
Member Author

greg0ire commented May 7, 2023

@mpdude please review :)

- Each type is now either final, abstract or an interface.
- The mappedBy attribute is no longer nullable and moved down the
  hierarchy.
- The inversedBy attribute is still nullable and also moved down the
  hierarchy.
- Code common to ManyToOneAssociationMapping and
  OneToOneOwningSideMapping is de-duplicated and moved up the hierarchy
- Code inside ToManyInverseSideMapping and ToManyOwningSideMapping comes
  from a trait to avoid duplication.
@derrabus derrabus added this to the 3.0.0 milestone May 7, 2023
@greg0ire greg0ire merged commit 05678dc into doctrine:3.0.x May 7, 2023
37 checks passed
@greg0ire greg0ire deleted the rework-class-hierarchy branch May 7, 2023 21:49
@mpdude
Copy link
Contributor

mpdude commented May 8, 2023

Let's hope that future merge-ups will not be too much of a headache with these changes. 🤞🏻

@greg0ire
Copy link
Member Author

greg0ire commented May 8, 2023

Not much more than previous ones I think… I'm mostly reworking new classes here 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants