Skip to content

DOCSP-36792: Add default database connection admonition #2888

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

Merged
merged 6 commits into from
Apr 24, 2024
Merged

DOCSP-36792: Add default database connection admonition #2888

merged 6 commits into from
Apr 24, 2024

Conversation

norareidy
Copy link
Contributor

@norareidy norareidy commented Apr 19, 2024

@norareidy norareidy changed the title DOCSP-36792: DB_CONNECTION configuration for facades DOCSP-36792: Add default database connection admonition Apr 19, 2024
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@norareidy norareidy marked this pull request as ready for review April 19, 2024 20:05
@norareidy norareidy requested a review from a team as a code owner April 19, 2024 20:05
@norareidy norareidy requested a review from GromNaN April 19, 2024 20:06
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, the DB facade can be used for any connection. Each model class specifies the connection it uses (for Eloquent builder), and the connection name can be specified before creating a query builder:

DB::connection('mongodb')->collection('users')->...

@@ -22,7 +22,7 @@ Configure Your MongoDB Connection

.. code-block:: php

'default' => env('DB_CONNECTION', 'mongodb'),
'default' => 'mongodb',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people want to update their default connection, it might be in the env var DB_CONNECTION in the .env file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should set the default connection to MongoDB even if the DB_CONNECTION variable is set to something else in .env. But I updated the admonition (in query-builder.txt) to give two options on how to set MongoDB as the connection, if that helps clarify

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remain convinced that this is the not the right way to change the default connection. @alcaeus any opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Laravel's default configuration uses env variables, and I agree that we shouldn't deviate too far from this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I changed these code snippets back to using env variables. Since we don't explain the .env file or its variables in the quick start, I made a ticket to add this info.

@norareidy norareidy requested a review from GromNaN April 22, 2024 19:43
@@ -22,7 +22,7 @@ Configure Your MongoDB Connection

.. code-block:: php

'default' => env('DB_CONNECTION', 'mongodb'),
'default' => 'mongodb',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Laravel's default configuration uses env variables, and I agree that we shouldn't deviate too far from this.

@@ -35,7 +35,7 @@ Configure Your MongoDB Connection
'connections' => [
'mongodb' => [
'driver' => 'mongodb',
'dsn' => env('DB_URI', '<connection string>'),
'dsn' => '<connection string>',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the connection string is likely to differ between environments (e.g. dev, staging, prod), we should stick with the Laravel default of providing this in an environment variable.

Suggested change
'dsn' => '<connection string>',
'dsn' => env('DB_URI', '<connection string>'),

Comment on lines 55 to 56
If MongoDB is not your application's default database, you can use the ``DB::connection()`` method
to specify a MongoDB connection. Pass a value of ``"mongodb"`` to the ``connection()`` method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should mention that one should pass the connection name (for which we use "mongodb" in our examples). Also, I suggest using a code snippet for this purpose:

Suggested change
If MongoDB is not your application's default database, you can use the ``DB::connection()`` method
to specify a MongoDB connection. Pass a value of ``"mongodb"`` to the ``connection()`` method.
If MongoDB is not your application's default database, you can use the ``DB::connection()`` method
to specify a MongoDB connection. Pass a the name of the connection to the ``connection()`` method:
.. code-block:: php
$connection = DB::connection('mongodb');

@norareidy norareidy requested a review from alcaeus April 23, 2024 14:39
@alcaeus alcaeus dismissed their stale review April 23, 2024 15:43

Changes addressed, deferring to Jérôme for final review.

@norareidy norareidy merged commit 6c5dff2 into mongodb:4.1 Apr 24, 2024
@norareidy norareidy deleted the DOCSP-36792-db-connection-setting branch April 24, 2024 14:53
This was referenced Apr 24, 2024
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.

5 participants