Skip to content

fix: handle failed database inserts in DatabaseHandler::push() - #103

Open
gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/database-handler-push-return-id
Open

gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/database-handler-push-return-id

Conversation

@gr8man

@gr8man gr8man commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Description

DatabaseHandler::push() assumed that Model::insert() always returns a job ID. In reality Model::insert() returns int|string|false and returns false when the insert cannot be performed (for example, when the statement fails or a beforeInsert callback aborts it).

Because the guard only checked $jobId === 0, a false return value fell through to QueuePushResult::success($jobId), whose parameter is declared as int. That raised a TypeError instead of returning a proper failure result, and the queue:jobPushed event was emitted for a job that was never stored.

Changes

  • src/Handlers/DatabaseHandler.php
    • Treat false, 0 and non-numeric values as a failed insert: emit queue:jobPushFailed and return QueuePushResult::failure('Failed to insert job into the database.').
    • Cast the accepted job ID to int before assigning it to the QueueJob entity and before calling QueuePushResult::success(), so the value always matches the declared types.

Testing

Added two cases to tests/DatabaseHandlerTest.php:

  • testPushWhenModelReturnsFalse() — a false return from the model produces a failed result with the expected error message and no job ID, does not emit queue:jobPushed and does emit queue:jobPushFailed.
  • testPushWhenModelReturnsZero() — 0 keeps the previous behaviour (failure result, jobPushFailed emitted, no jobPushed).

Both tests use an event-listener reset in tearDown() so the events of one test cannot leak into another.

Verified locally on PHP 8.5 with PHPUnit 11.5:

phpunit ....... OK (232 tests, 659 assertions, 27 skipped)
phpstan ....... [OK] No errors
psalm ......... No errors found
php-cs-fixer .. 0 of 75 files would be fixed

This branch has not been deployed

No deployments
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.

1 participant