feat(queue): added queue infrastructure, DB-backed jobs, worker, wiri… #3971
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a database-backed asynchronous queue: message types, handlers, transport, worker, CLI entrypoint, DB schema/migration for Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Bus as MessageBus
participant Transport as DatabaseTransport
participant DB as Database
participant Worker as Worker
participant Handler as Handler
Client->>Bus: dispatch(message, queue, headers)
Bus->>Bus: message->toArray() / JSON encode
Bus->>Transport: enqueue(payload, headers, queue)
Transport->>DB: INSERT INTO faqjobs (...)
DB-->>Transport: job_id
Transport-->>Bus: job_id
Bus-->>Client: job_id
Worker->>Transport: reserve(queue)
Transport->>DB: SELECT ... FOR UPDATE / UPDATE delivered_at
DB-->>Transport: job {id, body, headers}
Transport-->>Worker: job
Worker->>Worker: decodeMessage(body) -> message instance
Worker->>Handler: invoke handler(message)
alt success
Worker->>Transport: acknowledge(job_id)
Transport->>DB: DELETE FROM faqjobs WHERE id=job_id
else failure
Worker->>Worker: increment attempts
Worker->>Transport: release(job_id, available_at, headers)
Transport->>DB: UPDATE faqjobs SET available_at=..., headers=...
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@bin/worker.php`:
- Around line 25-26: The second require in bin/worker.php is pointing to the
wrong location (it currently references "phpmyfaq/src/libs/autoload.php");
update that require statement to load the autoload.php from the correct path
"phpmyfaq/src/autoload.php" so the script includes the actual autoloader instead
of the non-existent libs/autoload.php.
- Around line 30-35: The container is being used before compilation: after
loading services with PhpFileLoader/FileLocator and before calling
$container->get('phpmyfaq.queue.worker'), call $container->compile() to run
compiler passes, resolve references and validate definitions; update the
sequence around ContainerBuilder,
PhpFileLoader->load('../phpmyfaq/src/services.php'), then $container->compile(),
and only then fetch the service and call $worker->run($maxJobs).
In `@phpmyfaq/src/phpMyFAQ/Queue/Handler/SendMailHandler.php`:
- Around line 31-36: The SendMailHandler::__invoke currently silently returns
when $this->callback is not callable which can drop queued mails; update
__invoke to detect missing or non-callable $this->callback and either throw a
clear RuntimeException (e.g., "No mail callback configured") or at minimum call
the logger to emit a warning before returning; apply the same change pattern to
IndexFaqHandler::__invoke and ExportHandler::__invoke so an absent callback
produces an observable error/warning rather than a silent no-op.
In `@phpmyfaq/src/phpMyFAQ/Queue/Transport/DatabaseTransport.php`:
- Around line 74-118: The reserve() method has a race where two workers can
SELECT the same job and both get true from $db->query() even if zero rows were
updated; fix by extending the database API: add an affectedRows(int|void) method
to the DatabaseDriver interface and implement it in all driver classes, then
change the logic in Queue\Transport\DatabaseTransport::reserve() to execute the
UPDATE (markDelivered) and check $db->affectedRows() (expecting 1) before
returning the job; if affectedRows() !== 1, continue the loop and try the next
row. Ensure the new method is used by reserve() (alongside existing methods like
configuration->getDb(), Database::getTablePrefix(), and decodeHeaders()) so the
SELECT/UPDATE claim is only successful when the UPDATE actually modified a row.
In `@phpmyfaq/src/phpMyFAQ/Queue/Worker.php`:
- Around line 54-69: The worker currently re-releases failed jobs indefinitely
in Worker::run (or the try/catch block that calls decodeMessage and looks up
$this->handlers), so add a retry counter and a max attempts policy: read and
increment a retry header (e.g., from the decoded message headers or job row),
compare against a configurable MAX_RETRIES constant, and if the count exceeds
the limit move the job to a dead-letter state (or delete/acknowledge it) via the
database transport instead of calling databaseTransport->release; if under the
limit increment the counter and call databaseTransport->release with the updated
headers and delay as now. Ensure the logic touches decodeMessage,
$this->handlers lookup, and the catch block that currently calls
databaseTransport->release so retries are tracked and terminal failures are
handled.
- Around line 76-89: The loop in run() currently exits when runOnce($queue)
throws; wrap the runOnce call in a try/catch that catches \Throwable, logs the
exception (including job/queue context) and continues the loop so one failing
job doesn't stop the batch; keep the existing behavior of breaking when runOnce
returns false, and increment $processed only when runOnce succeeds; reference
run() and runOnce() and use the class logger (or error_log if no logger
property) to record the exception details.
- Around line 91-115: The decodeMessage method returns a stdClass envelope when
the declared message class exists but does not implement QueueMessageInterface,
which causes the handler lookup (which expects the message class via
$message::class) to never match and trigger infinite retries; update
decodeMessage (and its use of QueueMessageInterface and the fromArray factory)
to throw a RuntimeException immediately when class_exists($messageClass) is true
but !is_subclass_of($messageClass, QueueMessageInterface::class), removing the
stdClass fallback so callers always receive either a proper
QueueMessageInterface instance (via ClassName::fromArray(payload)) or an
explicit error.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php`:
- Around line 575-591: The CREATE TABLE SQL in Migration420Alpha (inside the
recorder->addSql call that creates %sfaqjobs) risks implicit TIMESTAMP defaults
on older MySQL; modify the SQL to prevent automatic ON UPDATE behavior by either
moving the created TIMESTAMP column before available_at (e.g., place created
TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP as the first TIMESTAMP column) or
explicitly set available_at with a safe default (e.g., available_at TIMESTAMP
NOT NULL DEFAULT '1970-01-01 00:00:00' or DEFAULT NULL if acceptable) so
available_at will not receive an implicit DEFAULT CURRENT_TIMESTAMP ON UPDATE;
update the SQL string in Migration420Alpha::up (the recorder->addSql invocation)
accordingly and keep PRIMARY KEY and INDEX definitions unchanged.
In `@phpmyfaq/src/services.php`:
- Around line 353-355: The queue handlers (services
phpmyfaq.queue.handler.send-mail, phpmyfaq.queue.handler.index-faq,
phpmyfaq.queue.handler.export) are registered with their classes only, but
SendMailHandler::__invoke, IndexFaqHandler::__invoke and ExportHandler::__invoke
require a $callback to perform work so these instances will be no-ops; fix by
wiring the required callback dependencies (or a concrete service implementing
the expected callback interface) into the service definitions when setting those
services, or refactor the handler constructors to accept injected collaborators
and populate the callback internally so the __invoke methods run real logic
instead of no-ops. Ensure you reference and update SendMailHandler,
IndexFaqHandler, and ExportHandler as needed to accept/receive the injected
callbacks or collaborators.
🧹 Nitpick comments (5)
bin/worker.php (1)
28-28:maxJobs = 0means "process until queue is empty" — consider documenting this.From the
Worker::runcode,maxJobs = 0processes all available jobs then stops (breaks whenrunOncereturns false). A brief usage comment or--helpoutput would clarify this for operators.phpmyfaq/src/phpMyFAQ/Queue/Transport/DatabaseTransport.php (2)
74-84: Unbounded SELECT inreserve()may become a performance bottleneck.The query on line 81 fetches all undelivered jobs for the queue with no
LIMIT. For a busy queue with a large backlog, this loads every pending row into memory even though only one job is needed. Consider fetching a small batch (e.g.,LIMIT 10) to reduce memory pressure, adapting the syntax per database adapter if needed.
37-69:enqueue()uses string interpolation for SQL — consider using prepared statements if available.Building the INSERT via
sprintfwith$db->escape()is the existing project pattern, but for thebodyfield (which contains arbitrary JSON payloads), parameterized queries would provide stronger protection against edge-case escaping failures. This is a defense-in-depth suggestion, not a blocker.tests/phpMyFAQ/Queue/Transport/DatabaseTransportTest.php (1)
15-98: Good coverage of the happy paths; consider adding tests forrelease()and edge cases.The three tests cover enqueue, reserve, and acknowledge well. Missing coverage:
release()method (no test at all)reserve()returningnullwhen the queue is emptyreserve()skipping jobs whoseavailable_atis in the futureThese gaps could mask regressions in the retry/release flow.
phpmyfaq/src/phpMyFAQ/Queue/DatabaseMessageBus.php (1)
46-48: Dead code:is_stringcheck is unreachable.With
JSON_THROW_ON_ERRORon line 44,json_encodewill throw a\JsonExceptionon failure instead of returningfalse. Theis_stringguard can never be reached.🧹 Remove unreachable check
$encodedBody = json_encode([ 'class' => $message::class, 'payload' => $payload, ], JSON_THROW_ON_ERROR); - if (!is_string($encodedBody)) { - throw new RuntimeException('Unable to encode queue message payload.'); - } - return $this->databaseTransport->enqueue($encodedBody, $headers, $queue);
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Database/PdoPgsql.php (1)
278-280:⚠️ Potential issue | 🟠 MajorPre-existing: PostgreSQL
LIMITsyntax is incorrect.Line 279 uses MySQL-style
LIMIT offset,countsyntax, but PostgreSQL requiresLIMIT count OFFSET offset. This will cause SQL errors when$rowcount > 0on a PostgreSQL database.Proposed fix
if (0 < $rowcount) { - $query .= sprintf(' LIMIT %d,%d', $offset, $rowcount); + $query .= sprintf(' LIMIT %d OFFSET %d', $rowcount, $offset); }
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Database/Mysqli.php`:
- Around line 320-326: The affectedRows() method currently returns (int)
$this->conn->affected_rows which can be -1 on error/no-op and differs from other
drivers (PDO, Pgsql, Sqlsrv) that return 0; update affectedRows() to normalize
negative values to 0 (e.g., check $this->conn->affected_rows and return 0 when
it's < 0, otherwise cast and return the value) so callers receive 0 for
error/no-op like the other driver implementations.
In `@phpmyfaq/src/phpMyFAQ/Queue/Handler/ExportHandler.php`:
- Around line 35-46: The ExportHandler::__invoke currently calls
Export::create(...)->generate(...) but discards the returned string and ignores
ExportMessage->userId; update __invoke in ExportHandler to capture the returned
content, then either persist it (e.g., save to a file or DB using an existing
storage service or repository) or deliver it to the user identified by
$message->userId (e.g., attach to a notification, enqueue a SendMail-like
action, or store a user-specific download record); ensure you reference
Export::create, Exporter::generate (the generate() return value),
ExportMessage->userId, and implement appropriate error handling/logging and
permissions check before persisting or delivering the generated content.
In `@phpmyfaq/src/phpMyFAQ/Queue/Handler/IndexFaqHandler.php`:
- Around line 42-59: The current guard in IndexFaqHandler (after calling
Faq::getFaq) allows the fallback "Access Denied" placeholder through because
Faq::getFaq always sets faqRecord['id'] and faqRecord['content']; update the
condition before calling Elasticsearch->index to ensure the record is a real,
active FAQ — e.g., check $faq->faqRecord['active'] === 1 (or truthy) and/or
ensure $faq->faqRecord['content'] !== Translation::get('msgAccessDenied') — so
only genuine, active FAQs from Faq::getFaq are indexed.
In `@phpmyfaq/src/phpMyFAQ/Queue/Transport/DatabaseTransport.php`:
- Around line 43-68: The enqueue flow in DatabaseTransport uses
Database::nextId() (SELECT MAX(id)+1) to generate ids which is not
concurrency-safe; change the faqjobs.id to be an AUTO_INCREMENT primary key and
stop calling nextId() in enqueue(), instead let the INSERT omit id and return
the generated id (or use the DB API's lastInsertId/insert_id); alternatively, if
schema changes aren't possible, wrap the nextId() + INSERT in a transaction with
a retry-on-duplicate-key loop (catch DB duplicate-key error, reselect next id
and retry) inside DatabaseTransport::enqueue to ensure correctness under
concurrent producers.
🧹 Nitpick comments (7)
tests/phpMyFAQ/Administration/FaqTest.php (1)
49-49: Prefernew stdClass()here for consistency.
createMock(stdClass::class)is unnecessary sincestdClasshas no methods to mock — you're only assigning properties directly. Lines 238 and 282 in this same file already usenew stdClass()for the same purpose.♻️ Suggested fix
- $mockResult = $this->createMock(stdClass::class); + $mockResult = new stdClass();phpmyfaq/src/phpMyFAQ/Queue/Handler/IndexFaqHandler.php (1)
42-49: Consider injectingFaq,Category, andElasticsearchinstead of direct instantiation.Direct
newcalls make unit-testing the happy path difficult — you can't mock the FAQ lookup or the ES client. If these were constructor-injected (or provided via a factory), the handler would be fully testable in isolation.phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php (1)
575-687: Column ordering differs between MySQL and the other engines.MySQL defines columns as
(…, created, available_at, delivered_at)while PostgreSQL, SQLite, and SQL Server use(…, available_at, delivered_at, created). While not a runtime issue, keeping column order consistent across engines simplifies maintenance and reduces the chance of copy-paste errors in future migrations.phpmyfaq/src/phpMyFAQ/Queue/Handler/ExportHandler.php (1)
40-40: Uncaught exception fromExport::createon unsupported format.
Export::createthrowsExceptionwhen$message->formatdoesn't match a supported mode ('json'or'pdf'). Since this runs inside a queue worker, an unhandled exception here could crash or stall the worker depending on the worker's error-handling strategy. Consider catching the exception and logging a meaningful error, or validating$message->formatbefore callingcreate.🛡️ Suggested defensive handling
+ use phpMyFAQ\Core\Log; + public function __invoke(ExportMessage $message): void { $faq = new Faq($this->configuration); $category = new Category($this->configuration); - $exporter = Export::create($faq, $category, $this->configuration, $message->format); - $exporter->generate( - categoryId: (int) ($message->options['categoryId'] ?? 0), - downwards: (bool) ($message->options['downwards'] ?? true), - language: (string) ($message->options['language'] ?? ''), - ); + try { + $exporter = Export::create($faq, $category, $this->configuration, $message->format); + $exporter->generate( + categoryId: (int) ($message->options['categoryId'] ?? 0), + downwards: (bool) ($message->options['downwards'] ?? true), + language: (string) ($message->options['language'] ?? ''), + ); + } catch (\Exception $e) { + // Log and let the worker mark the job as failed rather than crashing + error_log(sprintf('ExportHandler failed: %s', $e->getMessage())); + throw $e; + } }phpmyfaq/src/phpMyFAQ/Queue/Transport/DatabaseTransport.php (1)
80-84: Unbounded SELECT inreserve()— consider addingLIMIT 1.The query fetches all undelivered jobs matching the queue, then iterates in PHP. Under high load with many pending jobs, this fetches unnecessary rows. Adding
LIMIT 1to the SQL (or a small batch size) would reduce memory use and I/O.Note that the PHP-side
whileloop with theaffectedRows()check is still needed for the race condition, but withLIMIT 1, the common case (single available job) becomes much cheaper. If the single row is lost to a race,reserve()would returnnulland the next poll would try again.♻️ Suggested change
$query = sprintf( - "SELECT id, queue, body, headers, available_at FROM %s WHERE queue = '%s' AND delivered_at IS NULL ORDER BY available_at ASC, id ASC", + "SELECT id, queue, body, headers, available_at FROM %s WHERE queue = '%s' AND delivered_at IS NULL AND available_at <= '%s' ORDER BY available_at ASC, id ASC", $table, $escapedQueue, + date('Y-m-d H:i:s'), );This would also push the
available_atfiltering into the query, eliminating the PHP-side time comparison (lines 91-96).tests/phpMyFAQ/Queue/WorkerTest.php (1)
124-153: Worker error logging usesecho/output — consider structured logging.
expectOutputRegex('/Queue worker error in run\(\)/')implies theWorker::run()method writes errors to stdout. For a background worker, structured logging (e.g., PSR-3LoggerInterface) would be more appropriate than raw output, enabling log aggregation and level-based filtering.This is fine for an initial implementation but worth revisiting.
phpmyfaq/src/phpMyFAQ/Queue/Handler/SendMailHandler.php (1)
33-40: Unhandled exceptions from
addTo()andsend()can throw\phpMyFAQ\Core\Exception(or\Exception). In a queue handler, an unhandled exception typically marks the job as failed without a meaningful log entry. Wrapping the mail operations in a try/catch — logging the failure and optionally re-throwing a domain-specific exception — gives the worker a chance to record the error and decide on retry/dead-letter behaviour.💡 Suggested improvement
public function __invoke(SendMailMessage $message): void { - $mail = new Mail($this->configuration); - $mail->addTo($message->recipient); - $mail->subject = $message->subject; - $mail->message = $message->body; - $mail->send(); + try { + $mail = new Mail($this->configuration); + $mail->addTo($message->recipient); + $mail->subject = $message->subject; + $mail->message = $message->body; + $mail->send(); + } catch (\Exception $e) { + throw new \RuntimeException( + sprintf('SendMailHandler failed for "%s": %s', $message->recipient, $e->getMessage()), + 0, + $e, + ); + } }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/phpMyFAQ/Captcha/Helper/BuiltinCaptchaAbstractHelperTest.php (1)
165-184:⚠️ Potential issue | 🟠 MajorTest codifies XSS-vulnerable behavior in
BuiltinCaptchaAbstractHelper::renderCaptcha().Lines 182–183 assert that unescaped
<script>tags and unencoded&characters pass through into the HTML output. The implementation embeds both$label(line 61) and$action(lines 75–79) directly viasprintf()without sanitization, making them vulnerable to stored/reflected XSS if user-controlled data reaches them.Fix in
phpmyfaq/src/phpMyFAQ/Captcha/Helper/BuiltinCaptchaAbstractHelper.php: applyhtmlspecialchars()to both parameters before embedding, then update assertions to expect the escaped output (e.g.,&instead of&,<script>instead of<script>).phpmyfaq/src/phpMyFAQ/Captcha/GoogleRecaptcha.php (1)
42-46:⚠️ Potential issue | 🟠 MajorURL-encode query parameters to prevent parameter injection.
Both the secret key and
$code(user-supplied) are interpolated into the URL without encoding. If$codecontains&or#, it can truncate or alter the query string, causing silent verification failures or unintended parameter injection.Proposed fix
$url = sprintf( 'https://www.google.com/recaptcha/api/siteverify?secret=%s&response=%s', - $this->configuration->get(item: 'security.googleReCaptchaV2SecretKey'), - $code, + urlencode((string) $this->configuration->get(item: 'security.googleReCaptchaV2SecretKey')), + urlencode($code), );
🤖 Fix all issues with AI agents
In `@tests/phpMyFAQ/Queue/WorkerTest.php`:
- Around line 155-182: The test shows jobs get acknowledged and deleted silently
when attempts exceed MAX_RETRIES; modify Worker::runOnce to emit a clear
dead-letter notification before calling DatabaseTransport::acknowledge (or
immediately after detecting attempts >= MAX_RETRIES) — for example call an
injected event dispatcher or logger (e.g., error_log or an EventEmitter) with
job id, queue name, attempts count and exception message; ensure Worker has an
injectable logger/dispatcher (or uses an existing one) so the dead-letter action
is recorded whenever Worker::runOnce determines a job should be permanently
removed after max retries.
🧹 Nitpick comments (6)
tests/phpMyFAQ/Command/McpServerCommandTest.php (1)
33-34:createMock→createStubwould be more semantically precise for doubles without expectations.In several places (e.g.,
$outputon lines 34, 56, 72), the mock has noexpects()calls configured. PHPUnit'screateStub()is the intended API for test doubles that only stub return values without verifying interactions. The#[AllowMockObjectsWithoutExpectations]class attribute masks this, but usingcreateStub()where no expectations exist would make test intent clearer and allow removing that attribute.This is a stylistic preference and the current code works correctly.
Also applies to: 55-56, 71-72
tests/phpMyFAQ/NetworkTest.php (1)
28-28: PrefercreateStub()when no expectations are verified.Since none of the tests call
expects()on this mock,createStub()is the more semantically appropriate PHPUnit method. Switching tocreateMock()then suppressing the resulting warning with#[AllowMockObjectsWithoutExpectations]adds ceremony without benefit. If the intent was to fix a test failure, the root cause may lie elsewhere.- $this->config = $this->createMock(Configuration::class); + $this->config = $this->createStub(Configuration::class);If
createStubis restored, the#[AllowMockObjectsWithoutExpectations]attribute on the class (line 28) can also be removed.Also applies to: 36-36
tests/phpMyFAQ/Helper/SearchHelperTest.php (1)
24-26: Consider keepingcreateStubwhen no call-count expectations are needed.
createStubis the semantically correct choice when you only need to configure return values without verifying invocation counts. Switching tocreateMockworks here only because#[AllowMockObjectsWithoutExpectations]silences the resulting warnings. If the intent was purely to fix test failures, the root cause may lie elsewhere.tests/phpMyFAQ/Queue/Handler/HandlersTest.php (2)
22-28: Smoke-test only — consider adding behavioral coverage forSendMailHandler::__invoke.This test only verifies that the constructor doesn't throw. It doesn't exercise
__invoke, so actual mail-sending logic (e.g.,Mail::send()) remains untested. A test that mocksaddTo/subject/message/sendcalls would be more valuable.
42-48: Same as above —ExportHandler::__invokeis not exercised.Consider adding a test that invokes the handler with an
ExportMessageand asserts the expected behavior (e.g., thatExport::createis called with the correct format and options).tests/phpMyFAQ/Queue/WorkerTest.php (1)
124-153: Worker error handling usesecho/
expectOutputRegex('/Queue worker error in run\(\)/')reveals the Worker writes errors to stdout. In a CLI worker context this works, but injecting aPsr\Log\LoggerInterfacewould make the output testable without output buffering and give operators control over log routing (syslog, file, stderr, etc.).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Database/Pgsql.php`:
- Around line 349-358: The lastInsertId() implementation calls pg_query(...) and
then pg_fetch_row(...) without checking for failure; when no sequence was used
pg_query can return false which causes a TypeError. Change lastInsertId() to
check if $result === false (or if pg_fetch_row returns false) and in that case
return "0" (or 0) to match other drivers; otherwise cast and return
(int)$row[0]; reference the lastInsertId() method, pg_query($this->conn, 'SELECT
lastval()') and pg_fetch_row($result) when adding the guard and early return.
In `@phpmyfaq/src/phpMyFAQ/Queue/Handler/ExportHandler.php`:
- Around line 64-67: The current is_dir() followed by mkdir() in ExportHandler
(variable $exportDir) has a TOCTOU race: change the check to attempt mkdir() and
only throw if mkdir fails and the directory still does not exist (e.g. if
(!is_dir($exportDir) && !mkdir($exportDir, 0o775, true) && !is_dir($exportDir))
{ throw new RuntimeException(...); }) so concurrent creators won't cause a false
failure; keep the same permissions and exception message and reference
$exportDir and the ExportHandler method where this logic lives.
In `@phpmyfaq/src/phpMyFAQ/Queue/Transport/DatabaseTransport.php`:
- Around line 78-94: The code pulls all undelivered jobs then filters
future-scheduled rows in PHP; modify the SQL in the $query (built with $table
and $escapedQueue) to include an available_at predicate (e.g. "AND available_at
<= NOW()" or compare to a passed timestamp) and add a LIMIT using the existing
$rowcount/pagination parameter, then remove the PHP-only skip based on
$availableAt (the while loop using $db->fetchArray($result)); keep using
$db->query and $db->fetchArray but ensure the query uses the tightened WHERE
(including delivered_at IS NULL AND available_at <= ...) and LIMIT to avoid
unbounded result sets.
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Queue/Handler/ExportHandler.php (1)
77-88: Wrap$mail->send()in a try/catch to prevent job failure after file is already written.If
send()throws, the export file has been persisted but the job is marked as failed. The user has the file but gets no notification, and the worker may retry (producing duplicate files). Catch the mail exception and log a warning instead.Proposed fix
if (is_string($email) && $email !== '') { - $mail = new Mail($this->configuration); - $mail->addTo($email); - $mail->subject = 'Your phpMyFAQ export is ready'; - $mail->message = sprintf( - 'Your %s export has been generated and is available for download: %s', - strtoupper($message->format), - $filename, - ); - $mail->send(); + try { + $mail = new Mail($this->configuration); + $mail->addTo($email); + $mail->subject = 'Your phpMyFAQ export is ready'; + $mail->message = sprintf( + 'Your %s export has been generated and is available for download: %s', + strtoupper($message->format), + $filename, + ); + $mail->send(); + } catch (\Exception $e) { + // Export file was written successfully; log mail failure but don't fail the job + error_log(sprintf('Export notification email failed for user %d: %s', $message->userId, $e->getMessage())); + } }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (1)
116-128:⚠️ Potential issue | 🟡 MinorPre-existing bug:
pg_result_status($result)on line 124 willTypeErrorif the query fails.The new assignment at line 122 is fine, but note that when
pg_query()returnsfalse(line 118 branch), execution falls through to line 124 wherepg_result_status($result)receivesfalse— which is aTypeErroron PHP 8.x since it expectsPgSql\Result. This is pre-existing (line 124 is unchanged), but worth fixing while you're here.Proposed fix
$result = pg_query($this->conn, $query); if (!$result) { $this->sqlLog .= $this->error(); } $this->lastResult = $result; + if ($result === false) { + return false; + } + if (pg_result_status($result) === PGSQL_COMMAND_OK) { return true; }
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Queue/Handler/ExportHandler.php`:
- Around line 77-88: Wrap the notification block in ExportHandler that
constructs Mail and calls $mail->send() in a try/catch so a thrown exception
from $mail->send() cannot bubble up and mark the export job as failed;
specifically catch \Throwable around the Mail creation/call (the $mail->send()
invocation on the Mail instance) and log the error with context (recipient
$email, $filename, $message->format) instead of rethrowing, ensuring the export
result remains considered successful even if sending the email fails.
🧹 Nitpick comments (2)
tests/phpMyFAQ/DatabaseTest.php (1)
14-33: Good improvement: temporary file isolation for database tests.The setUp/tearDown lifecycle with
tempnam()is a clean approach for test isolation. Two minor observations:
- SQLite may create auxiliary files (e.g.,
<file>-journal,<file>-wal,<file>-shm) alongside the main database file. The tearDown only removes the main file, so these could accumulate in the temp directory.- The
Sqlite3connections opened in tests are never explicitly closed beforetearDownruns. On some platforms this could prevent file deletion.♻️ Optional: clean up auxiliary files and close connections
protected function tearDown(): void { parent::tearDown(); if (isset($this->sqliteTestFile) && is_file($this->sqliteTestFile)) { `@unlink`($this->sqliteTestFile); + // Clean up SQLite auxiliary files + foreach (['-journal', '-wal', '-shm'] as $suffix) { + $aux = $this->sqliteTestFile . $suffix; + if (is_file($aux)) { + `@unlink`($aux); + } + } } }phpmyfaq/src/phpMyFAQ/Queue/Handler/ExportHandler.php (1)
50-53: Direct instantiation ofFaq,Category,Export, andAll collaborators are
new'd inside__invoke, making it impossible to inject test doubles. If the project moves toward broader test coverage for handlers, consider injecting these as constructor dependencies or using a factory.
1 participant
…ng, and tests
Summary by CodeRabbit
New Features
Database
Tests
Chores