PHPORM-81 implement `mongodb` driver for batch #2904

GromNaN · 2024-04-23T20:07:59Z

Checklist

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

GromNaN · 2024-05-02T17:55:12Z

tests/Bus/MongoBatchRepositoryTest.php

+                $_SERVER['__progress.count']++;
+            })
+            ->then(function (Batch $batch) {
+                $_SERVER['__then.batch'] = $batch;


We can't use $this because the closure is serialized.

How would $this be used here even if it was supported?

Offhand, is laravel/serializable-closure used internally for serialization? AFAIK, the stock serialize() function doesn't support closures on its own.

All the $_SERVER['something'] could have been replaced by $this->data['something']. But that's not allowed by the package you mentioned.

https://github.com/laravel/framework/releases/tag/v10.37.2

alcaeus

I'm not sure about assuming that any string passed will be a valid ObjectId. A possible workaround would be to try and create an ObjectId instance, but leave the value as-is if it isn't valid. That way, the operation will either succeed (if such a record exists) or be a no-op (which would be the assumed behaviour for any batch that doesn't exist). As is, passing a string that isn't ObjectId-like would result in an exception, which IMO is not desirable.

alcaeus · 2024-05-06T07:32:15Z

src/Bus/MongoBatchRepository.php

+    public function get($limit = 50, $before = null): array
+    {
+        if (is_string($before)) {
+            $before = new ObjectId($before);


Beware that this may throw an exception if the string is not a well-formed ObjectId. Not sure if that's something we want to consider, but I figured I'd point it out.

In the store method, the _id is not set, so only ObjectId can be inserted.

I wanted to point this out as theoretically users could call the method with any string. No objection to assuming valid ObjectIDs are passed and leaving this as-is.

src/Bus/MongoBatchRepository.php

alcaeus · 2024-05-06T07:34:55Z

src/MongoDBQueueServiceProvider.php

+        if (! isset($config['collection']) && isset($config['table'])) {
+            trigger_error('Since mongodb/laravel-mongodb 4.4: Using "table" option for the queue is deprecated. Use "collection" instead.', E_USER_DEPRECATED);
+            $config['collection'] = $config['table'];
+        }


jmikola

Forgive me for any ignorant questions.

docs/queues.txt

jmikola · 2024-05-06T17:14:50Z

src/Bus/MongoBatchRepository.php

+
+// Extending DatabaseBatchRepository is necessary so methods pruneUnfinished and pruneCancelled
+// are called by PruneBatchesCommand
+class MongoBatchRepository extends DatabaseBatchRepository implements PrunableBatchRepository


Is there any reason for inconsistent use of "Mongo" and "MongoDB" as class prefixes? This isn't the first time I've noticed it, and I understand that there may be historical inconsistencies in the project, but this PR introduces two new classes with inconsistent names.

I also named MongoLock and MongoStore with the same unsaid naming rule, following the naming examples of MongoConnector, MongoQueue ...

src/Bus/MongoBatchRepository.php

jmikola · 2024-05-06T17:34:35Z

src/MongoDBQueueServiceProvider.php

@@ -51,6 +54,15 @@ protected function registerFailedJobServices()
     */
    protected function mongoFailedJobProvider(array $config): MongoFailedJobProvider
    {
-        return new MongoFailedJobProvider($this->app['db'], $config['database'], $config['table']);
+        if (! isset($config['collection']) && isset($config['table'])) {


Should it be an outright exception to specify both table and collection? As-is, I assume table would just be ignored.

Yes, table will be ignored if collection is set. Allowing both make migration easier as you can specify both during transition.

jmikola · 2024-05-06T17:37:08Z

tests/Bus/MongoBatchRepositoryTest.php

+                $_SERVER['__progress.count']++;
+            })
+            ->then(function (Batch $batch) {
+                $_SERVER['__then.batch'] = $batch;


How would $this be used here even if it was supported?

Offhand, is laravel/serializable-closure used internally for serialization? AFAIK, the stock serialize() function doesn't support closures on its own.

jmikola · 2024-05-07T00:23:36Z

CHANGELOG.md

@@ -4,6 +4,8 @@ All notable changes to this project will be documented in this file.
 ## [4.4.0] - unreleased

 * Support collection name prefix by @GromNaN in [#2930](https://github.com/mongodb/laravel-mongodb/pull/2930)
+* Add `mongodb` driver for Batching by @GromNaN in [#2904](https://github.com/mongodb/laravel-mongodb/pull/2904)
+* Rename queue option `table` to `collection`


Do you also need to note deprecating expire for retry_after?

jmikola · 2024-05-07T00:29:59Z

src/Bus/MongoBatchRepository.php

+    #[Override]
+    public function transaction(Closure $callback): mixed
+    {
+        return $this->connection->transaction(fn () => $callback());


Is there any significance to wrapping $callback in an additional closure?

GromNaN changed the title ~~PHPORM-81 implement mmongodb driver for batch~~ PHPORM-81 implement mongodb driver for batch Apr 25, 2024

GromNaN force-pushed the PHPORM-81 branch 2 times, most recently from 82ff224 to 7264181 Compare May 2, 2024 15:12

GromNaN marked this pull request as ready for review May 2, 2024 17:53

GromNaN requested review from a team as code owners May 2, 2024 17:53

GromNaN requested a review from jmikola May 2, 2024 17:53

GromNaN commented May 2, 2024

View reviewed changes

GromNaN force-pushed the PHPORM-81 branch from 0a4746d to bd20e72 Compare May 2, 2024 17:56

rustagir self-requested a review May 2, 2024 17:58

GromNaN force-pushed the PHPORM-81 branch from bd20e72 to 9d8e48d Compare May 2, 2024 18:02

GromNaN requested a review from alcaeus May 3, 2024 12:29

GromNaN added 8 commits May 3, 2024 14:30

PHPORM-81 implement mmongodb driver for batch

96daece

wip

98d9d6e

First test

8002982

Import Laravel tests

18beedb

-

f6d031d

Update docs and review option names

7690a0c

Adjust constraint for PendingBatch::progress

21fde62

https://github.com/laravel/framework/releases/tag/v10.37.2

Update changelog

73527f2

GromNaN force-pushed the PHPORM-81 branch from 0ea06f0 to 73527f2 Compare May 3, 2024 12:33

GromNaN changed the base branch from 4.3 to 4.4 May 3, 2024 13:05

alcaeus reviewed May 6, 2024

View reviewed changes

Merge branch '4.4' into PHPORM-81

6370763

jmikola reviewed May 6, 2024

View reviewed changes

GromNaN added 2 commits May 6, 2024 20:33

Doc fix

a56dd57

Add getUTCDateTime using Carbon::now

3de8412

jmikola reviewed May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPORM-81 implement `mongodb` driver for batch #2904

PHPORM-81 implement `mongodb` driver for batch #2904

GromNaN commented Apr 23, 2024 •

edited

GromNaN May 2, 2024

jmikola May 6, 2024

GromNaN May 6, 2024

alcaeus left a comment

alcaeus May 6, 2024

GromNaN May 6, 2024

alcaeus May 6, 2024 •

edited

alcaeus May 6, 2024

jmikola left a comment

jmikola May 6, 2024

GromNaN May 6, 2024

jmikola May 6, 2024

GromNaN May 6, 2024

jmikola May 6, 2024

jmikola May 7, 2024

jmikola May 7, 2024

PHPORM-81 implement mongodb driver for batch #2904

Are you sure you want to change the base?

PHPORM-81 implement mongodb driver for batch #2904

Conversation

GromNaN commented Apr 23, 2024 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PHPORM-81 implement `mongodb` driver for batch #2904

PHPORM-81 implement `mongodb` driver for batch #2904

GromNaN commented Apr 23, 2024 •

edited

alcaeus May 6, 2024 •

edited