Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PHPORM-174 Add doc for cache and locks #2909
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
PHPORM-174 Add doc for cache and locks #2909
Changes from 8 commits
1940776
1e42c9d
2a958da
e639b47
e934c53
5d884e1
a4ead2c
830ebd4
aa1dba8
7d3637d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I didn't pick up on this when reviewing the original PRs, so forgive me for asking this late: where does the database name come from? Are you relying on whatever is configured through the corresponding
connection
option (for both this andlock_collection
)?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 database name is set in the database connection configuration. Do you think we should add an option to change the database name for cache and lock?
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.
You can also consider allowing a namespace for the
collection
andlock_collection
options, and parsing out a database name by exploding on a dot character (no issue since it's prohibited in each part per Naming Restrictions.Otherwise, I'm fine keeping this as-is and revisiting if/when someone asks for further customization.
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.
Tracking this feature: PHPORM-176
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.
Should
collection
and variouslock
options be specified here?Also, is there any significance to the line break between
default
andstores
in the configuration array? I believe you're already emphasizing thedefault
line, so I don't think a line break is necessary for readability.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.
Here, I rely on the default values. Do you think I should make a note of that?
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.
Calling out your reliance on default values makes sense to me.
I was mostly concerned with
collection
. The MongoLock constructor requires a value for its Collection parameter, and I didn't see where the default gets supplied.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.
"connection" is required, "collection" has a default value which is
cache
. These 2 words are so similar that they can easily be confused.The default is applied in the service provider.
laravel-mongodb/src/MongoDBServiceProvider.php
Line 51 in f95eb66