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.
May be necessary for the CI.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
| @@ -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.
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.
| @@ -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.
string|list<string> would probably work better here.
There was a problem hiding this comment.
| @@ -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.
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.
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.
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.