Fix fatal Error when a Log is pushed without required fields - #51
Fix fatal Error when a Log is pushed without required fields#51claudear wants to merge 1 commit into
Conversation
`Logger::addLog()` validates a log with `empty($log->getAction()) || ...`,
but the required properties on `Log` are typed with no default. Reading
one before it is set is a fatal PHP `Error`
("Typed property Utopia\Logger\Log::$action must not be accessed before
initialization"), thrown from inside the getter before the guard's own
`throw new Exception('Log is not ready to be pushed.')` can run.
Because `Error` does not extend `Exception`, callers following the
documented `@throws Exception` contract never catch it, so a log that is
missing a field crashes the request it was meant to report on.
Default `$type`, `$message`, `$version`, `$environment` and `$action` to
an empty string so the existing `empty()` validation behaves as intended
and the readable getters keep their `string` return contract.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile SummaryThe PR initializes five required
Confidence Score: 5/5The PR appears safe to merge, with incomplete logs rejected before reaching an adapter and complete logs retaining their existing behavior. The new empty-string defaults satisfy the getters’ string contracts and are rejected by the existing readiness guard, while the tests cover both failure and success paths. Important Files Changed
Reviews (1): Last reviewed commit: "Fix fatal Error on unpopulated Log requi..." | Re-trigger Greptile |
CI status
This failure is pre-existing and unrelated to this change — Happy to add a separate PR that skips/marks the LogOwl e2e test when the endpoint is unreachable, if that is wanted — kept out of this one to keep the fix minimal. |
Fix Confidence: 93/100The failure was reproduced verbatim by a test before any code change, and the same test passes after the one-line-per-field default. The fix keeps the |
Problem
Sentry CLOUD-3PSQ — 1271 events:
Logger::addLog()opens with a readiness guard:But
Logdeclares its required fields as typed properties with no default (protected string $action;,src/Logger/Log.php:53) and the constructor only initializes$timestamp. Evaluatingempty($log->getAction())therefore reads an uninitialized typed property, which on PHP >= 8.1 is a fatalErrorthrown from insidegetAction()— before the guard's ownExceptioncan ever be reached.Since
Errordoes not extendException, callers that follow the documented@throws Exceptioncontract aroundaddLog()don't catch it, so the failure escapes the logging layer and takes down the request/worker it was supposed to be reporting on. The same fatal can fire from the adapters (Sentry::push()'transaction' => $log->getAction(), plusAppSignal,LogOwl,Raygun).$type,$message,$versionand$environmenthave the identical defect;$actionsurfaces in the title only because it is the first condition evaluated.Fix
Default the five required string properties to
'', soempty()behaves as the existing validation already assumes and callers get the intendedException('Log is not ready to be pushed.'). The getters keep theirstringreturn contract, so this is not a breaking change.Tests (TDD)
New
tests/unit/LoggerTest.php, written before the fix. Againstmainit reproduces the reported error verbatim:Coverage:
Log->addLog()throwsException('Log is not ready to be pushed.')Logmissing only the action -> sameExceptionLogreturn''instead of fatalingLogstill reaches the adapter and returns its statusVerified locally:
composer test-unit-> 8 tests / 54 assertions OK;composer lint(pint) passes;composer check(phpstan level max) reports no errors.🤖 Generated with Claude Code