-
Notifications
You must be signed in to change notification settings - Fork 0
Draft notes after a first review of the full code #3
Conversation
@@ -38,7 +38,7 @@ | |||
</testsuite> | |||
</testsuites> | |||
<php> | |||
<env name="MONGODB_URI" value="mongodb://mongodb/" /> | |||
<env name="MONGODB_URI" value="mongodb://127.0.0.1:27017/?serverSelectionTimeoutMS=100"/> |
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.
May be necessary for the CI.
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 believe it was CI that used the mongodb
name, but in that case I'd rather have a specific file for CI and have a version that works with a normal setup for local testing.
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.
mongodb
is the name of the container in docker-compose.
https://github.com/GromNaN/laravel-mongodb-private/blob/master/docker-compose.yml#L27
The env var is actually updated in CI:
https://github.com/GromNaN/laravel-mongodb-private/blob/master/.github/workflows/build-ci.yml#L97
@@ -1179,6 +1179,7 @@ class Message extends Model | |||
|
|||
### Authentication | |||
|
|||
// Could be simplified if we get rid of the custom PasswordResetServiceProvider |
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.
If we don't need the custom provider, I'm all for it.
@@ -38,7 +38,7 @@ | |||
</testsuite> | |||
</testsuites> | |||
<php> | |||
<env name="MONGODB_URI" value="mongodb://mongodb/" /> | |||
<env name="MONGODB_URI" value="mongodb://127.0.0.1:27017/?serverSelectionTimeoutMS=100"/> |
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 believe it was CI that used the mongodb
name, but in that case I'd rather have a specific file for CI and have a version that works with a normal setup for local testing.
@@ -27,6 +28,7 @@ protected function getPayload($email, $token) | |||
*/ | |||
protected function tokenExpired($createdAt) | |||
{ | |||
// Could be removed if Carbon was natively suported? |
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.
If the writes are handled through the Model
class, we could indeed get rid of it once we support Carbon. Otherwise, we're forced to handle this here. I'm not too familiar with this logic and haven't touched this part yet ("it just works"), but happy to simplify things where possible.
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.
@@ -112,7 +112,8 @@ public function getSchemaBuilder() | |||
* | |||
* @return Database | |||
*/ | |||
public function getMongoDB() | |||
// Should be named getDatabase ? | |||
public function getDatabase() |
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.
Yes, I don't like getMongoDB
as a name. We can introduce getDatabase
and deprecate the getMongoDB
method in 4.0 for later removal.
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.
@@ -826,15 +849,12 @@ public function pull($column, $value = null) | |||
/** | |||
* Remove one or more fields. | |||
* | |||
* @param mixed $columns | |||
* @param array|string $columns |
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.
string|list<string>
would probably work better here.
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.
@@ -933,9 +950,10 @@ protected function compileWheres(): array | |||
foreach ($wheres as $i => &$where) { | |||
// Make sure the operator is in lowercase. | |||
if (isset($where['operator'])) { | |||
$where['operator'] = strtolower($where['operator']); | |||
$where[' operator'] = strtolower($where['operator']); |
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 believe this change was done by accident.
if (Str::startsWith($operator, '$')) { | ||
$operator = substr($operator, 1); | ||
} | ||
// I need to understand this hack, why only when $boolean is not provided? |
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.
That's a good question 🤷♂️
@@ -4,6 +4,7 @@ | |||
|
|||
use Illuminate\Database\Query\Grammars\Grammar as BaseGrammar; | |||
|
|||
// Why override this class? Nothing is different from the base class. |
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.
If we can get away with not having these classes, I'm all for it. The less code we need to override the better it is.
When I was attempting to fix the weird auto-conversion of keys I noticed that there's a bunch of code that has moved in Laravel since this was written, so we may want to follow up the initial cleanup work with a more significant effort to update the code to the current Laravel specification and remove duplication as much as possible.
Closing as tickets or PRs have been created. |
@alcaeus I read the code and noted some remarks that may lead to new tickets.