Skip to content

[Fix] Format exception to string in failed jobs #1961

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 3 commits into from
Feb 23, 2020

Conversation

divine
Copy link
Contributor

@divine divine commented Feb 14, 2020

Format exception to string in failed job so it can be saved correctly.

See: https://github.com/laravel/framework/blob/6.x/src/Illuminate/Queue/Failed/DatabaseFailedJobProvider.php#L59

Closes: #1800, #1524, #1480

@divine
Copy link
Contributor Author

divine commented Feb 14, 2020

We might actually do something like this:

    /**
     * Log a failed job into storage.
     * @param string $connection
     * @param string $queue
     * @param string $payload
     * @param  \Exception  $exception
     * @return \MongoDB\BSON\ObjectId|null
     */
    public function log($connection, $queue, $payload, $exception)
    {
        $failed_at = Carbon::now()->getTimestamp();

        $exception = (string) $exception;

        return $this->getTable()->insertGetId(compact(
            'connection', 'queue', 'payload', 'exception', 'failed_at'
        ));
    }

However insertGetId returns @inheritdoc which is int but in our case it's MongoDB\BSON\ObjectId

See:
https://github.com/jenssegers/laravel-mongodb/blob/master/src/Jenssegers/Mongodb/Eloquent/Builder.php#L66
and
https://github.com/jenssegers/laravel-mongodb/blob/master/src/Jenssegers/Mongodb/Query/Builder.php#L572

@divine divine requested a review from Smolevich February 14, 2020 06:01
@divine divine force-pushed the fix_failed_job_exception branch from 9cbc524 to d5a7bac Compare February 14, 2020 06:15
@Giacomo92
Copy link

Add tests, see here

@divine
Copy link
Contributor Author

divine commented Feb 14, 2020

Add tests, see here

There is no complete tests for jobs so I'm not sure if we should do that now?

Also function log doesn't return insert id which should do that at first, then we could work out on adding good tests.

I think better example for tests would be https://github.com/laravel/framework/tree/6.x/tests/Queue

@Giacomo92
Copy link

Yes, Laravel tests are more complete. It would be awesome if you add tests for all job features.

@divine
Copy link
Contributor Author

divine commented Feb 14, 2020

Yes, Laravel tests are more complete. It would be awesome if you add tests for all job features.

Even making a breaking changes is fine? 😀

@Smolevich Smolevich merged commit 5438286 into mongodb:master Feb 23, 2020
@divine divine deleted the fix_failed_job_exception branch February 23, 2020 17:19
mnphpexpert added a commit to mnphpexpert/laravel-mongodb that referenced this pull request Sep 2, 2024
[Fix] Format exception to string in failed jobs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Job exception missing from failed_jobs table when using Laravel Queues
3 participants