DOCSP-35934: databases and collections #2816

rustagir · 2024-04-02T21:02:05Z

Checklist

Add tests and ensure they pass
Add an entry to the CHANGELOG.md file
Update documentation for new features

GromNaN · 2024-04-03T13:40:34Z

docs/fundamentals/database-collection.txt

+
+       'mongodb' => [
+           'driver' => 'mongodb',
+           'dsn' => env('DB_URI', '<connection string>'),


The config/dabase.php file is committed in the git repository of each project. The MongoDB DSN usually contains credentials, it should be in the .env file.
The 2nd argument of env() is the default value. Typically mongodb://localhost:27017 for the MongoDB DSN.

The application can contain a .env.example but in local the developers must create the file .env. For production, there is many ways to set the environment variables, from the .env file to server or container config.

I believe this is content that will be better suited for the Connection Guide

This is wrong. When a "dsn" is provided, the database name is read from the "dsn", and the "database" config is ignored.

Your example should look like this, so we don't bother with env vars.

'mongodb' => [ 'driver' => 'mongodb', 'dsn' => 'mongodb://localhost:27017/animals, ],

When a "dsn" is provided, the database name is read from the "dsn", and the "database" config is ignored.

Outside the scope of this PR, but is that something we should consider changing in some future release. The "auth database" component in the DSN should ideally never be specified since it was made obsolete by the authSource URI option (as I explained in symfony-cli/symfony-cli#458 (comment)).

docs/fundamentals/database-collection.txt

…5934-db-coll

jmikola · 2024-04-10T16:09:30Z

docs/fundamentals/database-collection.txt

+:ref:`laravel-eloquent-migrations` section in the Schema Builder guide.
+
+To drop a collection idiomatically, the Laravel ``Schema`` facade provides the
+``drop()`` and ``dropIfExists()`` methods. The following example


@GromNaN: Since dropping a nonexistent collection is a NOP in PHP, is there any practical difference between these two methods? Does it make sense to talk about both of them given the driver behavior?

So the doc should tell both methods have the same effect and they exist for compatibility with the SQL Eloquent.

We could remove the collection existence check from this method?

laravel-mongodb/src/Schema/Builder.php

Lines 87 to 92 in 95c5b4f

public function dropIfExists($table)

{

if ($this->hasCollection($table)) {

$this->drop($table);

}

}

Correct. I see the Blueprint::drop() method calls Collection::drop(). That NOPs if the namespace doesn't exist.

docs/fundamentals/database-collection.txt

jmikola · 2024-04-10T16:15:30Z

docs/fundamentals/database-collection.txt

+   The MongoDB PHP driver reuses the same connection when
+   you create two clients with the same connection string. There is no
+   overhead in using two connections for two distinct databases, so you
+   do not need to optimize your connections.


I'll note that if users expect to change the database name for separate connections, that would result in the driver not re-using the same connection since the connection string (i.e. dsn option) will technically differ.

This relates to my suggestion in https://github.com/mongodb/laravel-mongodb/pull/2816/files#r1559726362 to encourage users not to specify any "auth database" in the connection string, which the Laravel library just so happens to use as a "default" database.

Thanks for the information! Since i modified the multiple connections example to specify the default db in the connection strings, should I remove this note then?

I'm asking @GromNaN above, but I very much dislike the example being presented here. If there's any way we can change this to avoid using the "auth database" URI component, I'd like to see it.

I can confirm that this paragraph is no longer relevant given the current state of the PR, but the fact that it's now creating multiple database connections is very disagreeable to me. This doesn't feel like something we should be conveying so simply in docs that some users are bound to copy/paste as reference.

The correct configuration should be to extract the database into a dedicated config:

'connections' => [ 'mongodb' => [ 'driver' => 'mongodb', 'dsn' => 'mongodb://localhost:27017/', 'database' => 'animals', ], 'mongodb_alt' => [ 'driver' => 'mongodb', 'dsn' => 'mongodb://localhost:27017/', 'database' => 'plants', ] ], ...

ccho-mongodb

Overall, the content looks good. As mentioned in my comments, I think there could be some structural changes that make the info more focused and easier to locate, but would be ok if you choose not to take them.
I'll also leave it up to you whether you want me to take another look.

docs/fundamentals/database-collection.txt

ccho-mongodb · 2024-04-10T18:09:47Z

docs/fundamentals/database-collection.txt

+To drop a collection idiomatically, the Laravel ``Schema`` facade provides the
+``drop()`` and ``dropIfExists()`` methods. The following example


Suggestion:
I think this sentence structure could be simplified and made more concise. E.g.

"The Laravel Schema facade offers the drop() and dropIfExists() methods to drop a collection."

Or, if you end up suggesting just one, maybe something like the following:

"You can drop a collection by calling the "drop()" method of the Laravel Schema facade."

docs/fundamentals/database-collection.txt

ccho-mongodb

LGTM, left a couple small suggestions.

ccho-mongodb · 2024-04-12T00:11:21Z

docs/fundamentals/database-collection.txt

+You specify a database name when configuring a connection in your
+application's ``config/database.php`` file. The ``connections`` property


Suggestion:

I think this sentence specifies a time ("when"), but I don't think it's connected to any sequence of instructions we provide. I think the following sentence could avoid associating it with an order:

You can configure the name of the database that a connection uses in your application's config/database.php configuration file.

ccho-mongodb · 2024-04-12T00:16:49Z

docs/fundamentals/database-collection.txt

+
+If your application contains multiple database connections and you want
+to store your model in a database other than the default, override the
+``$connection`` property when creating a ``Model`` class.


Suggestion:

I think it's only necessary to mention where to make the change.

Suggested change

``$connection`` property when creating a ``Model`` class.

``$connection`` property in your ``Model`` class.

ccho-mongodb · 2024-04-12T00:19:07Z

docs/fundamentals/database-collection.txt

+
+When you create model class that extends
+``MongoDB\Laravel\Eloquent\Model``, {+odm-short+} stores the model data
+in a collection with a name that is the snake case plural form of your


Suggestion:

No change necessary, but I just thought this wording might flow better:

Suggested change

in a collection with a name that is the snake case plural form of your

in a collection with a name formatted as the snake case plural form of your

ccho-mongodb · 2024-04-12T00:23:56Z

docs/fundamentals/database-collection.txt

+
+   Flower::where('name', 'Water Lily')->get()
+
+If you are not able to accomplish your operation by using an Eloquent


(correcting my prior suggestion)

Suggested change

If you are not able to accomplish your operation by using an Eloquent

If you are unable to accomplish your operation by using an Eloquent

ccho-mongodb · 2024-04-12T00:26:58Z

docs/fundamentals/database-collection.txt

+model, you can access a collection by using a facade to access the query
+builder class ``DB`` and calling the ``collection()`` method. The


Suggestion:

I think it can be more concise to use the "DB facade" nomenclature which the Laravel docs use.

Suggested change

model, you can access a collection by using a facade to access the query

builder class ``DB`` and calling the ``collection()`` method. The

model, you can access the query builder by calling the ``collection()`` method on the ``DB`` facade. The

ccho-mongodb · 2024-04-12T00:30:15Z

docs/fundamentals/database-collection.txt

+   Model Class guide.
+
+We generally recommend that you use the Eloquent ORM to access a collection
+implicitly for code readability and maintainability. The following


Suggestion:
I think there's only one way to reference collections by using Eloquent ORM (through the models), so it could be confusing to include "implicitly":

Suggested change

implicitly for code readability and maintainability. The following

for code readability and maintainability. The following

docs/fundamentals/database-collection.txt

jmikola · 2024-04-12T16:03:02Z

docs/fundamentals/database-collection.txt

+   The MongoDB PHP driver reuses the same connection when
+   you create two clients with the same connection string. There is no
+   overhead in using two connections for two distinct databases, so you
+   do not need to optimize your connections.


I'm asking @GromNaN above, but I very much dislike the example being presented here. If there's any way we can change this to avoid using the "auth database" URI component, I'd like to see it.

I can confirm that this paragraph is no longer relevant given the current state of the PR, but the fact that it's now creating multiple database connections is very disagreeable to me. This doesn't feel like something we should be conveying so simply in docs that some users are bound to copy/paste as reference.

docs/fundamentals/database-collection.txt

…5934-db-coll

* DOCSP-35934: db and coll * add sections * small fixes * CC and DBX feedback * tech review fixes and CC suggestions * small fixes

DOCSP-35934: db and coll

7397ca6

rustagir requested a review from a team as a code owner April 2, 2024 21:02

rustagir requested a review from jmikola April 2, 2024 21:02

ccho-mongodb added the docs label Apr 2, 2024

GromNaN requested review from GromNaN and removed request for jmikola April 3, 2024 10:10

GromNaN requested changes Apr 3, 2024

View reviewed changes

add sections

9c313fd

rustagir requested a review from a team as a code owner April 10, 2024 14:50

rustagir added 2 commits April 10, 2024 10:55

small fixes

c8263c0

Merge branch '4.1' of github.com:mongodb/laravel-mongodb into DOCSP-3…

cfcf100

…5934-db-coll

rustagir requested a review from GromNaN April 10, 2024 15:11

jmikola reviewed Apr 10, 2024

View reviewed changes

docs/fundamentals/database-collection.txt Outdated Show resolved Hide resolved

jmikola reviewed Apr 10, 2024

View reviewed changes

ccho-mongodb approved these changes Apr 10, 2024

View reviewed changes

CC and DBX feedback

2cbf27e

rustagir requested a review from ccho-mongodb April 11, 2024 19:38

ccho-mongodb approved these changes Apr 12, 2024

View reviewed changes

jmikola reviewed Apr 12, 2024

View reviewed changes

GromNaN requested changes Apr 15, 2024

View reviewed changes

docs/fundamentals/database-collection.txt Outdated Show resolved Hide resolved

rustagir added 3 commits April 15, 2024 16:14

tech review fixes and CC suggestions

37c3940

Merge branch '4.1' of github.com:mongodb/laravel-mongodb into DOCSP-3…

d3e9691

…5934-db-coll

small fixes

95ed1df

rustagir requested a review from GromNaN April 15, 2024 20:17

GromNaN approved these changes Apr 15, 2024

View reviewed changes

rustagir merged commit 93727cd into mongodb:4.1 Apr 15, 2024
15 checks passed

mongodb-php-bot mentioned this pull request Apr 15, 2024

Merge 4.1 into 4.2 #2862

Merged

rustagir mentioned this pull request Apr 15, 2024

Add toc changes for db and coll docs #2863

Merged

mongodb-php-bot mentioned this pull request Apr 15, 2024

Merge 4.1 into 4.2 #2864

Merged

This was referenced Apr 15, 2024

Merge 4.2 into 4.3 #2865

Merged

Merge 4.2 into 4.3 #2866

Merged

	public function dropIfExists($table)
	{
	if ($this->hasCollection($table)) {
	$this->drop($table);
	}
	}

		To drop a collection idiomatically, the Laravel ``Schema`` facade provides the
		``drop()`` and ``dropIfExists()`` methods. The following example

		You specify a database name when configuring a connection in your
		application's ``config/database.php`` file. The ``connections`` property

	``$connection`` property when creating a ``Model`` class.
	``$connection`` property in your ``Model`` class.

	in a collection with a name that is the snake case plural form of your
	in a collection with a name formatted as the snake case plural form of your


		Flower::where('name', 'Water Lily')->get()

		If you are not able to accomplish your operation by using an Eloquent

	If you are not able to accomplish your operation by using an Eloquent
	If you are unable to accomplish your operation by using an Eloquent

		model, you can access a collection by using a facade to access the query
		builder class ``DB`` and calling the ``collection()`` method. The

	model, you can access a collection by using a facade to access the query
	builder class ``DB`` and calling the ``collection()`` method. The
	model, you can access the query builder by calling the ``collection()`` method on the ``DB`` facade. The

	implicitly for code readability and maintainability. The following
	for code readability and maintainability. The following

DOCSP-35934: databases and collections #2816

DOCSP-35934: databases and collections #2816

Uh oh!

Conversation

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!