Editing the Doctrine section to improve accuracy and readability #6455
Conversation
| @@ -916,8 +924,81 @@ a ``name`` field and the associated getter and setter functions. | |||
| Relationship Mapping Metadata | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
|
|
|||
There was a problem hiding this comment.
I felt like a more-detailed explanation of the difference between OneToMany and ManyToOne was warranted here, since we're dealing with readers who may not be familiar with those terms (and who may be confused by the fact that both terms can be used to describe the same relationship).
There was a problem hiding this comment.
I really like that you switched the order to show ManyToOne first. I wrote this original spot, but it's bothered me for a long time that we showed OneToMany first (I hadn't had a chance to change it).
|
@natechicago thanks for this nice contribution and for making it not too long so we can fully review it. Thanks! |
| makes sense in the application for each ``Category`` to hold an array of | ||
| ``Product`` objects. | ||
| Doctrine does not *require* that the "one" side of a one-to-many relationship | ||
| hold a collection of its "many" related entities. But in the context of our |
There was a problem hiding this comment.
Not to split hairs, but using the present subjunctive is grammatically appropriate here.
There was a problem hiding this comment.
Why? The verb refers to the "the one side" subject of this sentence, doesn't it? And this is in its singular form.
We could also change it to "Doctrine does not require the "one" side of a one-to-many relationship to hold [...]". What do you think?
There was a problem hiding this comment.
You're right that the subject here is "the one side" (3rd person, singular). However, the 3rd person singular conjugation of the verb "hold" in the present subjunctive is hold, not holds. http://www.verbix.com/webverbix/English/hold.html But since this has caused some unnecessary confusion, I'll change the phrasing per your second suggestion.
Changes from code review.
Changes from code review.
| @@ -916,8 +924,81 @@ a ``name`` field and the associated getter and setter functions. | |||
| Relationship Mapping Metadata | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
|
|
|||
| To relate the ``Category`` and ``Product`` entities, start by creating a | |||
| ``products`` property on the ``Category`` class: | |||
| In our example, each category can be associated with *many* products, while | |||
There was a problem hiding this comment.
One last comment: In the docs we never used the first person perspective. So I would change this to "In this example, [...]".
There was a problem hiding this comment.
Good catch. Thanks for the thorough review on this PR!
|
@natechicago I just left one last minor comment. Other this looks really great to me. 👍 |
Changes from code review.
| application, it makes sense for each ``Category`` object to hold a collection | ||
| of ``Product`` objects. However, if we had decided against adding a ``$products`` | ||
| property to the ``Category`` class, then the ``Product`` entity's ``inversedBy`` | ||
| metadata would have to be omitted. |
There was a problem hiding this comment.
This is the first spot where I've found your explanation and wording anything but wonderful. Here, the first sentence isn't completely clear (and as you mentioned, it's very important that we're clear). When you say "does not require" - it's not clear if the mapping is not required (this is what you meant) or if the mapping is required, but "holding a collection of its many related entities" is somehow the optional part.
This is a tricky spot: as it's a "choice", and it's not clear why I would make one choice or another. Let's recommend that they do make both sides of the relationship (as we're doing now), and make this more of a light note - e.g. btw, you don't have to map the OneToMany side, but if you're unsure, do it.
There was a problem hiding this comment.
I've tried to reword this a bit, but fear it may still be a little too clunky. Feel free to have at it yourself during the merge :)
|
@natechicago I left a few comments for you. But this is AWESOME. Can you also read all the other pages on the docs? ;) You're making some changes (and more) that I've wanted to make for awhile. Very clear explanation in a spot that is commonly difficult for beginners. |
Changes from code review.
|
@weaverryan Thanks for the feedback and encouragement! |
|
👍 |
|
Thank you @natechicago. |
…ability (natechicago) This PR was squashed before being merged into the 2.3 branch (closes #6455). Discussion ---------- Editing the Doctrine section to improve accuracy and readability The changes here add some important details to "Relationship Mapping Metadata" section. I've also reorganized it a little bit. In this section, beginners are led through the process of associating products and categories. These entities are related in a many-to-one relationship. However, the documentation previously introduced the somewhat-confusing and *optional* information (the OneToMany mapping of a collection of product objects onto the category class) prior to introducing the more straight-forward and *mandatory* information (the ManyToOne mapping of a category object onto the product class). The changes in this PR introduce the ManyToOne mapping before the OneToMany mapping, so that the overall flow of information is from the most important to the less important. The changes also make it explicitly clear that the OneToMany mapping is optional. | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | 2.3 | Fixed tickets | n/a Commits ------- 1e06da5 Editing the Doctrine section to improve accuracy and readability
The changes here add some important details to "Relationship Mapping Metadata" section. I've also reorganized it a little bit.
In this section, beginners are led through the process of associating products and categories. These entities are related in a many-to-one relationship. However, the documentation previously introduced the somewhat-confusing and optional information (the OneToMany mapping of a collection of product objects onto the category class) prior to introducing the more straight-forward and mandatory information (the ManyToOne mapping of a category object onto the product class).
The changes in this PR introduce the ManyToOne mapping before the OneToMany mapping, so that the overall flow of information is from the most important to the less important. The changes also make it explicitly clear that the OneToMany mapping is optional.