Skip to content

Changed failed_at field as ISODate #2607

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 8 commits into from
Sep 11, 2023
Merged

Conversation

BehroozBvk
Copy link
Contributor

@BehroozBvk BehroozBvk commented Sep 6, 2023

Fix #2605

Changed failed_at field to ISODate, When config/queue.php failed.driver is mongodb.

@@ -28,7 +28,7 @@ public function log($connection, $queue, $payload, $exception)
'connection' => $connection,
'queue' => $queue,
'payload' => $payload,
'failed_at' => Carbon::now()->getTimestamp(),
'failed_at' => new \MongoDB\BSON\UTCDateTime(now()),
Copy link
Member

Choose a reason for hiding this comment

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

Noted that Carbon::now() allows for mocking time in tests, so I presume that now() does the same, so we shouldn't rely on creating a new UTCDateTime instance without a time given.

@alcaeus
Copy link
Member

alcaeus commented Sep 7, 2023

@BehroozBvk there are some coding standards issues. You can fix these by running ./vendor/bin/phpcbf src/Queue/Failed/MongoFailedJobProvider.php.

@GromNaN
Copy link
Member

GromNaN commented Sep 7, 2023

You can add a test in QueueTest with something like this:

        // Log a failed job
        Carbon::setTestNow('2019-01-01 00:00:00');
        $provider->log('test_connection', 'test_queue', 'test_payload', new Exception('test_exception'));

        $failedJob = Queue::getDatabase()->table(Config::get('queue.failed.table'))->first();

        $this->assertSame('test_connection', $failedJob['connection']);
        $this->assertSame('test_queue', $failedJob['queue']);
        $this->assertSame('test_payload', $failedJob['payload']);
        $this->assertEquals(new UTCDateTime(now()), $failedJob['failed_at']);
        $this->assertStringStartsWith('Exception: test_exception in ', $failedJob['exception']);

@GromNaN GromNaN self-requested a review September 7, 2023 08:32
@BehroozBvk
Copy link
Contributor Author

BehroozBvk commented Sep 8, 2023

@alcaeus Do you have any special settings for phpcbf? Because I used this command and it automatically corrected the codes.

$ ./vendor/bin/phpcbf packages/laravel-mongodb/tests/QueueTest.php

@GromNaN
Copy link
Member

GromNaN commented Sep 8, 2023

@alcaeus Do you have any special settings for phpcbf? Because I used this command and it automatically corrected the codes.

$ ./vendor/bin/phpcbf packages/laravel-mongodb/tests/QueueTest.php

The command must run from inside the laravel-mongodb directory to take the phpcs.xml config.

'failed_at' => Carbon::now()->getTimestamp(),
'exception' => (string) $exception,
]);
$this->getTable()->insert(
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert this unrelated style changes please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used phpcs.xml.dist in the package exactly this time. I hope everything is ok.

./vendor/bin/phpcbf --standard=phpcs.xml.dist src/Queue/Failed/MongoFailedJobProvider.php 
./vendor/bin/phpcbf --standard=phpcs.xml.dist tests/QueueTest.php

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.

Thank you. I reverted some style changes.

@GromNaN GromNaN merged commit cc005bf into mongodb:master Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants