Skip to content

Fix fatal Error when a Log is pushed without required fields - #51

Open
claudear wants to merge 1 commit into
mainfrom
fix/clo-3psq-uninitialized-log-action
Open

Fix fatal Error when a Log is pushed without required fields#51
claudear wants to merge 1 commit into
mainfrom
fix/clo-3psq-uninitialized-log-action

Conversation

@claudear

Copy link
Copy Markdown

Problem

Sentry CLOUD-3PSQ — 1271 events:

Error: Typed property Utopia\Logger\Log::$action must not be accessed before initialization

Logger::addLog() opens with a readiness guard:

if (empty($log->getAction()) || empty($log->getEnvironment()) || ...) {
    throw new Exception('Log is not ready to be pushed.');
}

But Log declares its required fields as typed properties with no default (protected string $action;, src/Logger/Log.php:53) and the constructor only initializes $timestamp. Evaluating empty($log->getAction()) therefore reads an uninitialized typed property, which on PHP >= 8.1 is a fatal Error thrown from inside getAction() — before the guard's own Exception can ever be reached.

Since Error does not extend Exception, callers that follow the documented @throws Exception contract around addLog() 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(), plus AppSignal, LogOwl, Raygun).

$type, $message, $version and $environment have the identical defect; $action surfaces in the title only because it is the first condition evaluated.

Fix

Default the five required string properties to '', so empty() behaves as the existing validation already assumes and callers get the intended Exception('Log is not ready to be pushed.'). The getters keep their string return contract, so this is not a breaking change.

Tests (TDD)

New tests/unit/LoggerTest.php, written before the fix. Against main it reproduces the reported error verbatim:

1) LoggerTest::testAddLogWithEmptyLogThrowsException
Failed asserting that exception of type "Error" matches expected exception "Exception".
Message was: "Typed property Utopia\Logger\Log::$action must not be accessed before initialization"

Coverage:

  • bare Log -> addLog() throws Exception('Log is not ready to be pushed.')
  • Log missing only the action -> same Exception
  • required getters on a fresh Log return '' instead of fataling
  • fully populated Log still reaches the adapter and returns its status

Verified 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

`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-apps

greptile-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown

Greptile Summary

The PR initializes five required Log string properties to empty strings so incomplete logs reach the existing readiness validation instead of triggering an uninitialized-property error.

  • Adds safe defaults for type, message, version, environment, and action.
  • Adds unit coverage for empty, partially populated, and complete logs, plus fresh-log getter behavior.

Confidence Score: 5/5

The 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

Filename Overview
src/Logger/Log.php Initializes required string fields to empty values, allowing the existing Logger::addLog() guard to reject incomplete logs with the documented exception.
tests/unit/LoggerTest.php Covers empty and partially populated rejection, fresh getter defaults, and successful dispatch of a complete log.

Reviews (1): Last reviewed commit: "Fix fatal Error on unpopulated Log requi..." | Re-trigger Greptile

@claudear

Copy link
Copy Markdown
Author

CI status

  • Linter (pint) — pass
  • CodeQL (composer check, phpstan level max) — pass
  • Tests 8.1 / 8.2 / 8.3 / nightly — unit suite passes on every version: OK (8 tests, 54 assertions). The job then fails in the e2e suite on LogOwlTest::testAdapter:
LogOwl push failed with fetch error: Could not resolve host: api.logowl.io

This failure is pre-existing and unrelated to this change — api.logowl.io no longer resolves from CI, so the LogOwl e2e test has been failing on every PR for months (e.g. run 28773564373 on verbose-type, which shows the identical Could not resolve host: api.logowl.io before any of this branch's changes). The Tests workflow has failed on all 12 of its most recent runs for the same reason.

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.

@claudear

Copy link
Copy Markdown
Author

Fix Confidence: 93/100

The 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 string return type of all getters, so it is not a source-breaking change, and phpstan level max plus pint pass in a CI-identical container. Slight uncertainty: any downstream consumer that relied on isset($log->action)-style reflection or on the fatal as an implicit signal would now see '' instead, and the e2e suite could not be run locally (it needs live provider credentials), though it is unaffected by this change since those tests populate every field.

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.

2 participants