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

Remove targetDocument autocompletion #2442

Merged

Conversation

malarzm
Copy link
Member

@malarzm malarzm commented May 4, 2022

This dodges a BC break, probably not impacting a lot of users but still a BC break. I use single reference as an example but same goes for EmbedOne. Currently

    /** @ODM\ReferenceOne() */
    private BaseDocument $referenceToAnywhere;

is a perfectly fine mapping that allows user to have a reference to any document (from ODM's perspective) as long as it extends BaseDocument (from PHP's perspective). With #2411 due to autocompletion for typed properties this would become (implicitly):

    /** @ODM\ReferenceOne(targetDocument=BaseDocument::class) */
    private BaseDocument $referenceToAnywhere;

which is wrong. This could be worked around by an explicit targetDocument=false in the mapping but that would then require us to widen targetDocument to allow |false. As we want to get 2.4.0 out of the door let's defer the decision and work to future us.

If we ever get to implementing this feature there's a number of edge cases we'll need to consider as well. All I've found so far involve having typed properties and discriminators (be it maps or anything else hinting discriminators may be in use). Hopefully these will be easier to spot with my next PR which will move autocompleting types before some mapping sanity checks.

@malarzm malarzm added the Bug label May 4, 2022
@malarzm malarzm added this to the 2.4.0 milestone May 4, 2022
@malarzm malarzm force-pushed the remove-assoc-one-mapping-autocomplete branch from ce0f0d6 to c711cea Compare May 4, 2022 21:25
@malarzm malarzm added Task and removed Bug labels May 4, 2022
@malarzm malarzm force-pushed the remove-assoc-one-mapping-autocomplete branch from c711cea to ac895f4 Compare May 4, 2022 21:47
@malarzm malarzm force-pushed the remove-assoc-one-mapping-autocomplete branch from ac895f4 to 904da94 Compare May 4, 2022 21:53
@malarzm malarzm merged commit 8f8f9fe into doctrine:2.4.x May 8, 2022
@malarzm malarzm deleted the remove-assoc-one-mapping-autocomplete branch May 8, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant