Add retry mechanism and more detailed logging for connection issues#1
Open
danbarker wants to merge 2 commits into
Open
Add retry mechanism and more detailed logging for connection issues#1danbarker wants to merge 2 commits into
danbarker wants to merge 2 commits into
Conversation
…rException in ConnectionFactory
) update() and doUpdate() are both written to accept either an array of values or a Criteria (doUpdate has an explicit `$updateValues instanceof Criteria` branch, and update()'s guard allows both). But basePreUpdate()/preUpdate() typed the parameter `array &$values`, so passing a Criteria fatals: TypeError: ModelCriteria::basePreUpdate(): Argument #1 ($values) must be of type array, Propel\Runtime\ActiveQuery\Criteria given This breaks the generated ON DELETE SET NULL emulation (doOnDeleteSetNull builds a Criteria and calls update()) for any model that does not get a behavior-generated basePreUpdate() override — QueryBuilder emits that override with an *untyped* `&$values`, so behavior-ful models (e.g. versionable) already work while timestampable-only / plain models hit the typed runtime method and 500. Drop the `array` hint so the runtime matches the generator (and upstream), letting both forms flow through to doUpdate(). Co-authored-by: Dan Barker <pat@yearbook.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3776f37. Configure here.
| error_log(sprintf('PDO Connection Error: DSN: %s, Error: %s, Code: %s', | ||
| preg_replace('/password=.*?;/', 'password=***;', $dsn), | ||
| $e->getMessage(), | ||
| $e->getCode() |
There was a problem hiding this comment.
Password logged when last DSN segment
High Severity
The new PDO connection error logging tries to mask credentials with preg_replace('/password=.*?;/', ...), but Propel DSNs often end with ;password=... and no trailing semicolon. In that common layout the pattern never matches, so the real password is written to error_log on connection failure.
Reviewed by Cursor Bugbot for commit 3776f37. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attempt to fix https://github.com/yearbook/ybmv3/issues/5581
Note
Medium Risk
Connection creation now delays up to ~200ms on failures and writes DSN-related details to error_log; retries could mask persistent misconfiguration briefly but do not change successful-path behavior.
Overview
Adds resilience and observability around Propel opening database connections, plus a small query hook typing fix.
Connection path:
ConnectionFactory::create()now retries up to 3 attempts (initial + 2 retries) when the adapter throwsAdapterException, with 100ms between tries. Each failed attempt is written toerror_log, and the final failure becomes aConnectionExceptionthat mentions the attempt count and preserves the last underlying error.PDO layer: On
PDOExceptionduring connect,PdoAdapterlogs DSN (with password masked), message, and code viaerror_logbefore wrapping inAdapterException.Unrelated fix:
ModelCriteria::basePreUpdate()/preUpdate()drop thearraytype hint on$valuesso behavior hooks can pass either an update array or aCriteria(e.g. ON DELETE SET NULL emulation), with PHPDoc updated accordingly.Reviewed by Cursor Bugbot for commit 3776f37. Bugbot is set up for automated code reviews on this repo. Configure here.