-
Notifications
You must be signed in to change notification settings - Fork 1.5k
DOCSP-37056 eloquent relationships #2747
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
DOCSP-37056 eloquent relationships #2747
Conversation
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.
a few small things but good work on this challenging material!
docs/eloquent-models.txt
Outdated
Eloquent models are part of the Laravel Eloquent object-relational | ||
mapping (ORM) framework that enable you to work with a database by using |
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.
S: the "that enable" is ambiguous
Eloquent models are part of the Laravel Eloquent object-relational | |
mapping (ORM) framework that enable you to work with a database by using | |
Eloquent models are part of the Laravel Eloquent object-relational | |
mapping (ORM) framework. These models enable you to work with a database by using |
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.
Good catch!
I'll use a slightly different structure than suggested since it's the framework rather than the models that relate to the second part.
|
||
To learn more about many to many relationships in Laravel, see | ||
`Many to Many <https://laravel.com/docs/{+laravel-docs-version+}/eloquent-relationships#many-to-many>`__ | ||
in the Laravel docs. |
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.
in the Laravel docs. | |
in the Laravel documentation. |
|
||
To learn more about one to many relationships, see | ||
`One to Many <https://laravel.com/docs/{+laravel-docs-version+}/eloquent-relationships#one-to-many>`__ | ||
in the Laravel docs. |
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.
in the Laravel docs. | |
in the Laravel documentation. |
the parent model instead of keeping foreign key references. This pattern | ||
when you must optimize for one or more of the following requirements: |
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.
the parent model instead of keeping foreign key references. This pattern | |
when you must optimize for one or more of the following requirements: | |
the parent model instead of keeping foreign key references. This pattern | |
is useful if you must optimize for one or more of the following requirements: |
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.
Thanks for catching the grammar issue. I think I forgot to change "This" -> "Use this". Rephrasing slightly to match the related fix.
- Keep associated data together in a single collection | ||
- Perform atomic updates on multiple fields of the document and the associated | ||
data | ||
- Reduce the number of reads required to fetch the data |
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.
S: based on the way the introduction to this list ends, I think you should change the format of these items
- Keep associated data together in a single collection | |
- Perform atomic updates on multiple fields of the document and the associated | |
data | |
- Reduce the number of reads required to fetch the data | |
- You must keep associated data together in a single collection | |
- You must perform atomic updates on multiple fields of the document and the associated | |
data | |
- You must reduce the number of reads required to fetch the data |
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.
Good idea. I decided to convert the verb to a noun by using the gerund.
I think this agrees better with the list introduction.
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.
A few small things but approving - re-request review if you feel it's necessary!
The following example class shows how to define a ``HasOne`` one to one | ||
relationship between a ``Planet`` and ``Orbit`` model. |
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.
S: this code example intro and the following one refer to different parts of the code to refer to the relationship as HasOne
vs by the method belongsTo()
. Perhaps standardize these to either refer to the relationship or the method that creates the relationship?
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.
Good catch, will standardize.
|
||
A many to many relationship consists of a relationship between two different | ||
model types in which one type of model record can be related to multiple | ||
records of the other type. |
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.
S: clarify that this relationship applies to both models
records of the other type. | |
records of the other type and (vice versa or something like that). |
The following section shows an example of how to create a many to many | ||
relationship between model classes. |
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.
S: concision
The following section shows an example of how to create a many to many | |
relationship between model classes. | |
The following examples demonstrates how to create a many to many | |
relationship between model classes. |
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.
Thanks for pointing this out, will use the standardized version (referenced in this comment) instead.
:dedent: | ||
|
||
The following sample code shows how ton instantiate a model for each class | ||
and add the relationship between them. Click the :guilabel:`View Output` |
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.
nit: View Output capitalization does not match the GUI VIEW OUTPUT
and add the relationship between them. Click the :guilabel:`View Output` | |
and add the relationship between them. Click the :guilabel:`View Output` |
The following example class shows how to define an ``embedsMany`` one to many | ||
relationship between a ``SpaceShip`` and ``Cargo`` model: |
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.
S; related to earlier comment - decide on a convention to describe this relationship.
The following example class shows how to define an ``embedsMany`` one to many | |
relationship between a ``SpaceShip`` and ``Cargo`` model: | |
The following example class shows how to define an ``EmbedsMany`` one to many | |
relationship between a ``SpaceShip`` and ``Cargo`` model: |
Or
The following example class shows how to define an ``embedsMany`` one to many | |
relationship between a ``SpaceShip`` and ``Cargo`` model: | |
The following example class shows how to use the ``embedsMany()`` method to define a one to many | |
relationship between a ``SpaceShip`` and ``Cargo`` model: |
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.
Using the standardized version mentioned in this comment.
$spaceship->name = 'The Millenium Falcon'; | ||
$spaceship->save(); | ||
|
||
$cargoSpice = new Cargo(); | ||
$cargoSpice->name = 'spice'; |
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.
Mixing fictions!
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.
Or fictions colliding!
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.
reapproving!
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.
We can consider adding unit tests to doc example, that's something do for PHPLIB: https://github.com/mongodb/mongo-php-library/blob/master/tests/DocumentationExamplesTest.php
docs/includes/eloquent-models/relationships/RelationshipController.php
Outdated
Show resolved
Hide resolved
docs/includes/eloquent-models/relationships/RelationshipController.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.
Don't forget to squash on merge.
* DOCSP-37056: Eloquent relationships
JIRA:
https://jira.mongodb.org/browse/DOCSP-37056
Staging:
Eloquent Models Landing Page
Eloquent Models Relationships
Notes for reviewers:
Checklist