feat!: rebuild timezone handling for PHP 8.3 and Symfony 7.4–8.1 - #21
Open
lunetics wants to merge 34 commits into
Open
feat!: rebuild timezone handling for PHP 8.3 and Symfony 7.4–8.1#21lunetics wants to merge 34 commits into
lunetics wants to merge 34 commits into
Conversation
Move production code into src/, add typed resolver and storage contracts, ship optional framework adapters, and harden diagnostics, persistence, packaging, documentation, and CI. BREAKING CHANGE: V2 removes the legacy guesser, event, provider, and validator APIs without a compatibility layer. See UPGRADE-2.0.md.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+220
to
+223
| $services->set('lunetics_timezone.resolver.stored_manual', StoredPreferenceTimezoneResolver::class) | ||
| ->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::MANUAL, $persistenceStrategy]) | ||
| ->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => 925, 'index' => 'stored_manual']); | ||
| $addToCatalog('stored_manual', 925); |
There was a problem hiding this comment.
stored_manual (925) and stored_browser (800) are hardcoded as literals here, but StoredPreferenceTimezoneResolver already defines MANUAL_PRIORITY = 925 and BROWSER_PRIORITY = 800 as named constants. If the constants are updated, the DI registration silently stays at the old numbers, letting priorities and documentation drift apart.
Suggested change
| $services->set('lunetics_timezone.resolver.stored_manual', StoredPreferenceTimezoneResolver::class) | |
| ->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::MANUAL, $persistenceStrategy]) | |
| ->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => 925, 'index' => 'stored_manual']); | |
| $addToCatalog('stored_manual', 925); | |
| $services->set('lunetics_timezone.resolver.stored_manual', StoredPreferenceTimezoneResolver::class) | |
| ->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::MANUAL, $persistenceStrategy]) | |
| ->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => StoredPreferenceTimezoneResolver::MANUAL_PRIORITY, 'index' => 'stored_manual']); | |
| $addToCatalog('stored_manual', StoredPreferenceTimezoneResolver::MANUAL_PRIORITY); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/LuneticsTimezoneBundle.php
Line: 220-223
Comment:
The priority values for `stored_manual` (925) and `stored_browser` (800) are hardcoded as literals here, but `StoredPreferenceTimezoneResolver` already defines `MANUAL_PRIORITY = 925` and `BROWSER_PRIORITY = 800` as named constants. If the constants are updated, the DI registration silently stays at the old numbers, letting priorities and documentation drift apart.
```suggestion
$services->set('lunetics_timezone.resolver.stored_manual', StoredPreferenceTimezoneResolver::class)
->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::MANUAL, $persistenceStrategy])
->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => StoredPreferenceTimezoneResolver::MANUAL_PRIORITY, 'index' => 'stored_manual']);
$addToCatalog('stored_manual', StoredPreferenceTimezoneResolver::MANUAL_PRIORITY);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+236
to
+239
| $services->set('lunetics_timezone.resolver.stored_browser', StoredPreferenceTimezoneResolver::class) | ||
| ->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::BROWSER, $persistenceStrategy]) | ||
| ->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => 800, 'index' => 'stored_browser']); | ||
| $addToCatalog('stored_browser', 800); |
There was a problem hiding this comment.
stored_browser resolver.
Suggested change
| $services->set('lunetics_timezone.resolver.stored_browser', StoredPreferenceTimezoneResolver::class) | |
| ->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::BROWSER, $persistenceStrategy]) | |
| ->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => 800, 'index' => 'stored_browser']); | |
| $addToCatalog('stored_browser', 800); | |
| $services->set('lunetics_timezone.resolver.stored_browser', StoredPreferenceTimezoneResolver::class) | |
| ->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::BROWSER, $persistenceStrategy]) | |
| ->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => StoredPreferenceTimezoneResolver::BROWSER_PRIORITY, 'index' => 'stored_browser']); | |
| $addToCatalog('stored_browser', StoredPreferenceTimezoneResolver::BROWSER_PRIORITY); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/LuneticsTimezoneBundle.php
Line: 236-239
Comment:
Same constant-vs-literal divergence for the `stored_browser` resolver.
```suggestion
$services->set('lunetics_timezone.resolver.stored_browser', StoredPreferenceTimezoneResolver::class)
->args([service(TimezonePreferenceStorageInterface::class), PreferenceSource::BROWSER, $persistenceStrategy])
->tag(TimezoneCompilerPass::RESOLVER_TAG, ['priority' => StoredPreferenceTimezoneResolver::BROWSER_PRIORITY, 'index' => 'stored_browser']);
$addToCatalog('stored_browser', StoredPreferenceTimezoneResolver::BROWSER_PRIORITY);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+109
to
+123
| public function write(Request $request, Response $response, TimezonePreference $preference): void | ||
| { | ||
| try { | ||
| $payload = json_encode(['v' => 1, 'timezone' => $preference->timezone->value(), 'source' => $preference->source->value, 'recorded_at' => $preference->recordedAt->format(\DateTimeInterface::ATOM)], JSON_THROW_ON_ERROR); | ||
| $payload64 = $this->base64UrlEncode($payload); | ||
| $value = $payload64.'.'.$this->base64UrlEncode(hash_hmac('sha256', $payload64, $this->key, true)); | ||
| if (strlen($value) > $this->maxEncodedSize) { | ||
| throw new \LengthException('Encoded timezone cookie exceeds its size limit.'); | ||
| } | ||
| $secure = $this->resolveSecure($request); | ||
| $response->headers->setCookie(Cookie::create($this->name, $value, $this->clock->now()->getTimestamp() + $this->maxAge, $this->path, $this->domain, $secure, $this->httpOnly, false, $this->cookieSameSite())); | ||
| } catch (\JsonException|\LengthException|\RuntimeException $failure) { | ||
| throw TimezoneStorageException::operationFailed('write', $failure); | ||
| } | ||
| } |
There was a problem hiding this comment.
TimezoneStorageException from resolveSecure() gets double-wrapped
TimezoneStorageException extends TimezoneException extends \RuntimeException, so when resolveSecure() throws new TimezoneStorageException('The configured timezone cookie requires a secure request.') on a non-HTTPS request with SameSite=None or __Host- naming, that exception is caught by catch (\RuntimeException $failure) and re-thrown as TimezoneStorageException::operationFailed('write', $failure). The top-level message becomes the generic "Timezone preference storage write failed.", demoting the specific root cause to the previous exception chain. Callers inspecting $e->getMessage() without traversing the chain get no actionable detail.
Consider excluding already-PersistenceFailureExceptionInterface instances from the re-wrapping: if ($failure instanceof PersistenceFailureExceptionInterface) { throw $failure; } at the top of the catch block.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Storage/CookieTimezoneStorage.php
Line: 109-123
Comment:
**`TimezoneStorageException` from `resolveSecure()` gets double-wrapped**
`TimezoneStorageException` extends `TimezoneException extends \RuntimeException`, so when `resolveSecure()` throws `new TimezoneStorageException('The configured timezone cookie requires a secure request.')` on a non-HTTPS request with `SameSite=None` or `__Host-` naming, that exception is caught by `catch (\RuntimeException $failure)` and re-thrown as `TimezoneStorageException::operationFailed('write', $failure)`. The top-level message becomes the generic "Timezone preference storage write failed.", demoting the specific root cause to the `previous` exception chain. Callers inspecting `$e->getMessage()` without traversing the chain get no actionable detail.
Consider excluding already-`PersistenceFailureExceptionInterface` instances from the re-wrapping: `if ($failure instanceof PersistenceFailureExceptionInterface) { throw $failure; }` at the top of the catch block.
How can I resolve this? If you propose a fix, please make it concise.The invalid-preference cleanup listener suppresses itself only via the marker attribute that BrowserTimezoneController sets, but the bundle itself never writes a manual preference — the application is the only designed producer for the stored_manual resolver, and its write() call sets no marker. When a request arrives with an invalid or expired stored preference (cookie secret rotation, max_age expiry) and the application persists a manual preference during that same request, the kernel.response cleanup replaces the fresh Set-Cookie with a delete-cookie or removes the session key — the submission is silently lost while the user sees a success response. The written marker now lives on TimezonePreferenceStorageInterface and both built-in storages set it inside write(), so the cleanup listener honours every writer. The controller keeps setting it as a safety net for custom storages, and the storage contract documentation states the duty. Regression tests pin the manual-write-survives-cleanup path for both storages.
ExportPolicyTest asserts the literal rule list of .gitattributes against a hardcoded copy of itself, so both sides of the assertion are the same artifact — development files that were never added to the list ship unnoticed, and scripts/no-dev-smoke.php (a CI-only script that throws when executed from an installed package) is in the archive today. Whenever a developer adds a new dev-only file without extending .gitattributes, every consumer receives it via composer install while the guard test stays green. The test now parses the actual 'git archive --worktree-attributes' output (minimal ustar reader, pax headers skipped) and asserts a top-level allowlist plus required distribution files; /scripts is export-ignored. It skips outside a git checkout.
The explicit non-public range list omits the well-known NAT64 prefix 64:ff9b::/96 (RFC 6052) and the 6to4 prefix 2002::/16, while PHP's FILTER_FLAG_NO_RES_RANGE does not know either — translation addresses that embed private IPv4 space pass both gates. An IPv6 client behind NAT64 (e.g. 64:ff9b::10.0.0.1) or a 6to4 address therefore reaches the third-party MaxMind reader that the resolver otherwise shields from non-public addresses, wasting a lookup on an address class that cannot geolocate meaningfully. Both prefixes are now part of explicitlyNonPublicRanges() and the non-public data provider covers them alongside the existing local-use NAT64 range.
The __Host- prefix rules are enforced in the storage constructor, in resolveSecure() and in the container configuration validation, but the weaker sibling prefix __Secure- is validated nowhere even though RFC 6265bis requires the Secure attribute for it as well. With name: '__Secure-tz' and secure: auto, a plain-HTTP request emits the cookie without the Secure attribute; browsers reject it, yet the endpoint answers 204, the change event fires and the profiler reports a successful write — nothing is ever persisted and the browser re-syncs on every page load. All three __Host- validation sites now also enforce explicit secure=true for __Secure- names, failing at construction/compile time instead of silently at runtime; docs and tests cover the new rule.
phpunit.xml.dist combines failOnDeprecation=true with ignoreDirectDeprecations=true, but a direct deprecation — first-party src/ code calling a deprecated Symfony/Twig API — is exactly the class this bundle can produce, so the gate can never fire for it (PHPUnit IssueFilter drops direct triggers before evaluation). When Symfony deprecates an API in 7.x/8.x that src/ still calls, the suite stays green on every matrix row and the breakage surfaces only as a fatal error in consumer applications after the next major. Only ignoreIndirectDeprecations remains (third-party-internal noise); the full suite passes without the direct filter on PHP 8.3 / Symfony 7.4.14.
reset() reads the constructor-initialized properties configuredDefault and configuredResolvers, but Symfony's DataCollector serialization contract carries only $data — on a collector loaded back out of profile storage those typed properties are uninitialized and reset() fatals with a typed-property Error. Any consumer calling reset() on a rehydrated instance — a profile replay path or code walking $profile->getCollectors() — crashes; today no in-tree caller does, which is why this stayed invisible. The properties are now declared with defaults and assigned in the constructor, so a rehydrated reset() falls back to empty configuration context instead of crashing; a test pins the round trip.
…face CurrentTimezoneProvider type-hints the concrete TimezoneExecutionContext while every other consumer binds TimezoneExecutionContextInterface, which is documented as a public contract. An application that aliases the interface to its own implementation pushes scopes onto the custom context (worker middleware, run() calls), but the provider keeps reading the bundle's concrete service whose stack stays empty — inside a Messenger handler getTimezone() silently returns the configured default instead of the stamped timezone. current() is now part of the interface and the provider depends on the interface, so a replaced context serves both sides consistently; the docs mention the new contract method.
The marker duty lives in the two built-in storages plus a docblock asking custom implementations to do the same — documentation-only enforcement — and the marker lands only on the Request object handed to write(), which the main-request cleanup listener never sees for subrequest writes. A custom storage written to directly, or any write performed on a subrequest (fragment renderer, forward()), still loses a freshly written preference to the invalid-preference cleanup on kernel.response whenever the incoming request carried a corrupt or expired record — both reproduced with executed probes during review. The bundle now decorates the configured storage (built-in or custom service id) with PreferenceWriteMarkingStorage, which marks the writing request AND the main request after every successful write; the built-in storages and the browser controller no longer set the marker themselves, the compiler pass validates the undecorated storage id, and regression tests cover the subrequest and decorated-custom-storage paths for both storages. Considered re-reading the storage before clearing instead — rejected because a cookie storage cannot observe its own response-bound write on a re-read.
The hand-maintained range list omits Teredo (2001::/32) and IPv6 benchmarking (2001:2::/48) although IpUtils is already imported and used in the same method, and Symfony's own IpUtils::PRIVATE_SUBNETS differs between versions (7.x dropped the documentation and benchmarking ranges that 6.4 lists), so neither source alone is complete. A client behind a Teredo tunnel or a benchmarking address passes both the filter_var gate and the explicit list and reaches the third-party MaxMind reader. explicitlyNonPublicRanges() now returns the union of IpUtils::PRIVATE_SUBNETS and the explicit special-purpose list, so the resolver rejects the superset regardless of the installed Symfony version; Teredo and benchmarking cases join the data provider.
The tar reader keeps only regular-file entries and the test skips on an empty shell_exec result, so a tracked symlink ships invisibly to the allowlist (git archives it with type flag '2'), a PAX long-path entry would be truncated to its 100-byte ustar name, and a failed or missing git binary turns the sole distribution gate into a skipped test that exits green. The parser now resolves PAX path records and GNU long names, includes symlinks as path-bearing entries, and fails loudly on unsupported entry types; git archive runs through proc_open with captured stderr and an asserted exit code, so only a genuine non-checkout environment skips.
The public-contracts listing still shows the execution-context interface without current() and the cookie-security sentence omits the __Secure- rule, both added to the code in the previous fix loop — installation.md was updated then, the plan document was not. Anyone porting against the plan document reads a stale contract. The interface listing now includes current() and the cookie rules name __Secure- alongside __Host-.
The provider's constructor binding to TimezoneExecutionContextInterface was fixed without a regression test, so a future revert to the concrete class would pass the suite silently. An application re-aliasing the interface would again observe the provider reading the bundle's concrete context while middleware pushes onto the custom one. A reflection test asserts the constructor parameter type is the interface.
The marking wrapper was wired as a second service id plus an interface alias, so the configured storage stayed reachable undecorated under its own class/service id — an app autowiring the concrete class (or its own custom storage class) bypassed the write tracking and reproduced the cleanup data-loss bug the wrapper exists to prevent; three independent review arms confirmed the bypass with compiled-container probes. Any preference write obtained via the concrete id set no marker, so the invalid-preference cleanup deleted the fresh value on kernel.response. The bundle now uses real Symfony service decoration on the configured id: every reference — interface alias, built-in class id, custom service id — resolves to PreferenceWriteMarkingStorage, and a concrete type-hint on an application class fails loudly at wiring time instead of silently losing the marker. Container tests pin the decorated alias shape and, per the same review, the provider's honouring of a replaced execution context; UPGRADE documents the injection contract.
The union delegated Teredo rejection to IpUtils::PRIVATE_SUBNETS, but symfony/http-foundation v6.4.0 — the exact floor the prefer-lowest CI job resolves — ships the constant without 2001::/32, so the new teredo test case fails deterministically on that job and Teredo addresses (which embed the client's private IPv4, RFC 4380) reach the MaxMind reader on old 6.4.x deployments. composer update --prefer-lowest downgrades to v6.4.0 and the suite goes red; production apps on the same floor leak the range. 2001::/32 joins the explicit list, which stays a superset of every range the resolver promises to reject on ANY supported Symfony version; PRIVATE_SUBNETS can now only ever widen the guard.
The checkout guard uses is_dir('.git'), which is false in a linked git
worktree where .git is a pointer file, and --worktree-attributes makes
the test validate the working tree's attribute rules instead of the
committed HEAD a release tag actually ships; a PAX header without a
path record also inherited a stale pending path from an earlier header.
A worktree checkout silently skips the only distribution check, and
uncommitted .gitattributes edits could green-light an archive that
differs from the released one.
The guard is file_exists() now, the archive is taken from committed
HEAD attributes, and an 'x' header without path= resets the pending
path instead of reusing a stale one.
The MANUAL-wins and storage-failure tests assert the written marker is absent while injecting a bare RecordingStorage into a controller that no longer marks — nothing under test could ever set the attribute, so the assertions became vacuous when the marker moved into the decorator. A regression that marks before the suppression early-returns or before a failing write would pass both tests unnoticed. Both tests now wrap their storage in PreferenceWriteMarkingStorage, so the absent-marker assertions guard the actual mechanism.
The marking decorator propagates every subrequest write to the main request, but a cookie write lives on the response it was handed. When a fragment-rendered controller writes, FragmentHandler discards that response's headers, so the fresh cookie never reaches the client while the propagated marker still suppresses the main-request cleanup — the stale cookie survives and the request self-heals only once the fragment stops writing. An expired or invalid preference cookie plus a preference write from a fragment controller reproduces it; forwarded controllers are unaffected because their response becomes the main one. Storage now declares its medium: ResponseScopedStorageInterface marks writes carried by a Response, and the decorator propagates the marker across requests only for lifecycle-scoped storage such as the session. Cookie storage refuses to clear a preference it has just written to the same response, so the forward case keeps its fresh cookie while the fragment case clears the stale one. Custom storage without the marker keeps the data-preserving lifecycle behaviour. Considered re-reading the storage before clearing — rejected because a cookie storage cannot observe its own response-bound write on a re-read.
|
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:
📝 WalkthroughWalkthroughThe pull request replaces the legacy bundle with a PHP 8.3 and Symfony 7.4–8.1 implementation. It adds ordered timezone resolution, persistence, browser synchronization, Symfony integrations, diagnostics, documentation, testing, packaging, and CI validation. ChangesTimezoneBundle 2.0
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BrowserTimezoneController
participant TimezonePreferenceStorage
participant TimezonePreferenceChangedEvent
Client->>BrowserTimezoneController: POST timezone preference
BrowserTimezoneController->>TimezonePreferenceStorage: read current preference
TimezonePreferenceStorage-->>BrowserTimezoneController: preference status
BrowserTimezoneController->>TimezonePreferenceStorage: write browser preference
TimezonePreferenceStorage-->>BrowserTimezoneController: successful write
BrowserTimezoneController->>TimezonePreferenceChangedEvent: dispatch preference change
BrowserTimezoneController-->>Client: HTTP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The DI registration hardcodes 925 and 800 although StoredPreferenceTimezoneResolver publishes MANUAL_PRIORITY and BROWSER_PRIORITY, so changing a constant leaves the container, the resolver catalog and the documentation silently disagreeing. Any future priority adjustment through the constants would not reach the tagged services at all. Both registrations and both catalog entries now read the constants, and a typed write failure from resolveSecure() is rethrown unchanged instead of being re-wrapped into the generic write-failure message — that path is unreachable behind the constructor invariants today, but the wrapper would hide an actionable configuration message if it ever fires. Both reported by the repository's greptile bot (P2).
lunetics
marked this pull request as ready for review
July 31, 2026 09:29
|
Too many files changed for review. ( Bypass the limit by tagging |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
Tests/Storage/CookieTimezoneStorageTest.php (2)
172-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
$clockparameter.
encodedCookie()receives$clockbut never uses it in the method body. Only$storageand$recordedAtare used.♻️ Proposed cleanup
- private function encodedCookie(CookieTimezoneStorage $storage, MutableClock $clock, \DateTimeImmutable $recordedAt): string + private function encodedCookie(CookieTimezoneStorage $storage, \DateTimeImmutable $recordedAt): string { $response = new Response(); $storage->write(Request::create('https://example.test'), $response, $this->preference('Europe/Berlin', PreferenceSource::BROWSER, $recordedAt));Update the two call sites in
testExpiredAndFutureDatedCookiesAreRejected()to drop the now-unused$clockargument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Storage/CookieTimezoneStorageTest.php` around lines 172 - 181, Remove the unused $clock parameter from encodedCookie(), then update both call sites in testExpiredAndFutureDatedCookiesAreRejected() to pass only the parameters the method uses: $storage and $recordedAt.Source: Linters/SAST tools
109-136: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a direct test for the response-scoped anti-clobber branch in
clear().
CookieTimezoneStorage::clear()refuses to clear a cookie that was just written to the sameResponse(hasFreshCookie()), per the documentedResponseScopedStorageInterfacecontract. No test in this file writes then clears on the sameResponseobject to verify the fresh cookie survives.Add a test that calls
write()andclear()with the sameResponseinstance, then asserts the written cookie value is still present and unmodified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Storage/CookieTimezoneStorageTest.php` around lines 109 - 136, Add a focused test for CookieTimezoneStorage::clear() using one shared Response: call write() followed by clear() on that same response, then assert the cookie retains the originally written value and attributes, confirming the fresh-cookie anti-clobber behavior.src/Storage/CookieTimezoneStorage.php (1)
177-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated envelope codec shared by
CookieTimezoneStorageandSessionTimezoneStorage. Both classes independently implement identicalparseAtom()canonical-timestamp parsing and identical envelope-shape validation (array_keys(...) !== ['v', 'timezone', 'source', 'recorded_at']plus per-field type checks). The root cause is the absence of a shared preference-envelope codec between the two storage backends.
src/Storage/CookieTimezoneStorage.php#L177-L192: extractparseAtom()and the envelope-shape/field-type validation (also used inread(), Lines 82-94) into a shared helper.src/Storage/SessionTimezoneStorage.php#L71-L102: extract the matchingencode()/decode()/parseAtom()logic into the same shared helper instead of maintaining a second copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Storage/CookieTimezoneStorage.php` around lines 177 - 192, Extract the duplicated preference-envelope codec into a shared helper used by both storage backends. In src/Storage/CookieTimezoneStorage.php lines 177-192 and its read() validation at lines 82-94, move parseAtom() and envelope key/type validation; in src/Storage/SessionTimezoneStorage.php lines 71-102, replace the local encode(), decode(), and parseAtom() implementations with the shared helper while preserving the existing envelope format and validation behavior.src/Resolver/LocaleMappingTimezoneResolver.php (1)
47-50: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider case normalization in
normalizeLocale.
normalizeLocalereplaces separators only. A configured keyde_denever matches request localede_DE, and the collision check at line 28 does not detect case-only duplicates. If you accept case-insensitive configuration, normalize case as well.♻️ Proposed change
private static function normalizeLocale(string $locale): string { - return str_replace('-', '_', $locale); + $normalized = str_replace('-', '_', $locale); + $parts = explode('_', $normalized); + $parts[0] = strtolower($parts[0]); + if (isset($parts[1])) { + $parts[1] = strtoupper($parts[1]); + } + + return implode('_', $parts); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Resolver/LocaleMappingTimezoneResolver.php` around lines 47 - 50, Update the normalizeLocale method to normalize locale casing in addition to replacing hyphens with underscores, ensuring configured keys and request locales match case-insensitively and allowing the collision check to detect case-only duplicates.src/Resolver/UserTimezoneResolver.php (1)
32-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider catching
\Throwablearound the user accessor calls.Both blocks catch
\Exceptiononly. A user entity proxy or a custom accessor can raise\Error(for exampleTypeErrorfrom a lazy Doctrine proxy). That error then escapes unwrapped and bypasses the resolver failure strategy.♻️ Proposed change
- } catch (\Exception $exception) { + } catch (\Throwable $exception) { throw TimezoneResolverException::userAccessorFailed($exception); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Resolver/UserTimezoneResolver.php` around lines 32 - 42, Update both user timezone accessor try/catch blocks in UserTimezoneResolver to catch \Throwable instead of \Exception, preserving the existing wrapping through TimezoneResolverException::userAccessorFailed($exception) for errors from either $user->getTimezone() or $this->accessor->getTimezoneForUser($user).Source: Linters/SAST tools
src/LuneticsTimezoneBundle.php (1)
214-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated resolver catalog build.
TimezoneCompilerPassrebuilds the catalog from the resolver tags and callsreplaceArgument(1, $resolverCatalog)onDebugTimezoneCommand(lines 127-145 of the pass). The catalog computed here is therefore always discarded. It also duplicates the sort and map logic, so the two implementations can diverge.Pass a placeholder argument here and keep the pass as the single source of ordering.
♻️ Proposed simplification
- /** `@var` list<array{name: string, priority: int}> $resolverCatalog */ - $resolverCatalog = []; - $catalogOrder = 0; - $addToCatalog = static function (string $name, int $priority) use (&$resolverCatalog, &$catalogOrder): void { - $resolverCatalog[] = ['name' => $name, 'priority' => $priority, 'order' => $catalogOrder++]; - };Remove every
$addToCatalog(...)call and theusort/array_mapat lines 283-284, then register the command with an empty catalog:- $services->set(DebugTimezoneCommand::class)->args([$config['default_timezone'], $resolverCatalog]); + $services->set(DebugTimezoneCommand::class)->args([$config['default_timezone'], []]);Also applies to: 283-284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/LuneticsTimezoneBundle.php` around lines 214 - 219, Remove the local resolver catalog construction in the command registration flow, including the `$addToCatalog` closure, all calls to it, and the `usort`/`array_map` transformations. Register `DebugTimezoneCommand` with an empty catalog placeholder so `TimezoneCompilerPass` remains the sole owner of resolver ordering and catalog population.Tests/Browser/timezone.test.mjs (1)
5-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore
globalThis.CustomEventalongside the other stubbed globals.
afterEachrestoresfetch,Intl, andwindowbut notCustomEvent, whichinstallBrowser()also overwrites at Line 19. Save and restore it the same way as the other globals, to avoid leaking the stub into other test files that may run in the same process and rely on the nativeCustomEvent.♻️ Proposed fix
const originalFetch = globalThis.fetch; const originalIntl = globalThis.Intl; const originalWindow = globalThis.window; +const originalCustomEvent = globalThis.CustomEvent; afterEach(() => { globalThis.fetch = originalFetch; globalThis.Intl = originalIntl; globalThis.window = originalWindow; + globalThis.CustomEvent = originalCustomEvent; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Browser/timezone.test.mjs` around lines 5 - 13, Capture the original globalThis.CustomEvent alongside the existing originalFetch, originalIntl, and originalWindow values, then restore globalThis.CustomEvent in afterEach after installBrowser() overwrites it.src/Controller/BrowserTimezoneController.php (1)
42-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Response::HTTP_REQUEST_ENTITY_TOO_LARGEfor status 413.Response::HTTP_PAYLOAD_TOO_LARGEis not defined in Symfony HttpFoundation 6.4–8.1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Controller/BrowserTimezoneController.php` around lines 42 - 43, Update the status constant in the oversized-content branch of BrowserTimezoneController so it uses the Symfony-defined Response::HTTP_REQUEST_ENTITY_TOO_LARGE constant instead of the unavailable payload-too-large constant, while preserving the 413 response behavior.Resources/public/timezone.js (1)
1-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a request timeout to the sync fetch call.
fetch()has no timeout. If the network stalls, the returned promise never resolves or rejects, and any caller awaiting it hangs indefinitely. Add anAbortControllerwith a timeout to bound the call.🔧 Proposed fix
-export function syncBrowserTimezone({endpoint, csrfToken, csrfHeader = 'X-CSRF-Token', storedBrowserTimezone}) { +export function syncBrowserTimezone({endpoint, csrfToken, csrfHeader = 'X-CSRF-Token', storedBrowserTimezone, timeoutMs = 5000}) { const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone; if (!timezone || timezone === storedBrowserTimezone) { return Promise.resolve(false); } const headers = {'Content-Type': 'application/json'}; if (csrfToken) { headers[csrfHeader] = csrfToken; } + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + return fetch(endpoint, { method: 'POST', headers, body: JSON.stringify({timezone}), credentials: 'same-origin', + signal: controller.signal, }).then((response) => { if (!response.ok) { return false; } window.dispatchEvent(new CustomEvent('lunetics:timezone-synced', {detail: {timezone}})); return true; - }).catch(() => false); + }).catch(() => false).finally(() => clearTimeout(timer)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Resources/public/timezone.js` around lines 1 - 25, Update syncBrowserTimezone to create an AbortController, pass its signal to the fetch call, and abort the request after a defined timeout so stalled requests settle; ensure the timeout is cleared when fetch completes while preserving the existing success and failure handling.Tests/EventListener/PreferenceDiagnosticsFlagsTest.php (1)
388-434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shared test double for the storage stubs.
The anonymous class at lines 390-413 and
RecordingCleanupStorageat lines 417-434 implement the same interface with near-identical bodies. A single configurable fake double that records calls and optional failures would remove the duplication and lettestSubRequestDoesNotInspectOrCleanCopiedInvalidPreferenceand the failure-strategy tests share one implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/EventListener/PreferenceDiagnosticsFlagsTest.php` around lines 388 - 434, Replace the anonymous storage class in storage() and RecordingCleanupStorage with one shared configurable TimezonePreferenceStorageInterface test double. Preserve absent reads, optional write/clear failures, and clear-call recording so testSubRequestDoesNotInspectOrCleanCopiedInvalidPreference and the failure-strategy tests can configure and reuse the same implementation.src/Bridge/WebProfiler/TimezoneDataCollector.php (1)
50-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the diagnostics key list once.
The same ten keys and their exact order appear in
collect(),reset(), andisDiagnostics().isDiagnostics()compares witharray_keys($data) !== [...], so any future key added tocollect()alone, or added in a different order, makesgetDiagnostics()throwLogicExceptionwhile the profiler panel renders. Extract one private constant for the key list and compare against it, or build the array through a single private factory used by bothcollect()andreset().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bridge/WebProfiler/TimezoneDataCollector.php` around lines 50 - 128, Define a single private constant for the ten diagnostics keys in their required order, then reuse it in collect(), reset(), and isDiagnostics() instead of repeating the literal key list. Ensure array construction and validation remain aligned so getDiagnostics() accepts data whenever the same keys are present in the defined order.src/Bridge/MaxMind/GeoIp2CityReader.php (1)
19-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNormalize malformed-IP failures.
GeoIp2\Database\Reader::city()throws\InvalidArgumentExceptionfor malformed IP addresses. Catch it and wrap it withTimezoneResolverException::maxMindLookupFailed($exception)so direct callers receive the normalized exception.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bridge/MaxMind/GeoIp2CityReader.php` around lines 19 - 28, Update GeoIp2CityReader::timezoneForIp to catch \InvalidArgumentException from the reader->city() lookup and wrap it with TimezoneResolverException::maxMindLookupFailed($exception), alongside the existing InvalidDatabaseException and BadMethodCallException handling. Preserve the current null result for AddressNotFoundException.Tests/Integration/BundleKernelSmokeTest.php (1)
44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack handler registration before calling
restore_exception_handler(). An unconditional call can remove PHPUnit's handler whenboot()fails before Symfony registers its handler. Do not useset_exception_handler(null)as a probe; it changes the handler stack, and the proposed double restore can remove PHPUnit's handler. Restore only when the test recorded a successful handler registration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Integration/BundleKernelSmokeTest.php` around lines 44 - 53, Update BundleKernelSmokeTest tearDown to restore the exception handler only when boot() recorded a successful Symfony handler registration; track that registration explicitly during boot and clear the tracking state after restoration. Remove the unconditional restore_exception_handler() call while preserving kernel shutdown and runtime-directory cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests.yml:
- Line 14: Update every actions/checkout@v4 step in the workflow, including the
four checkout steps referenced in the review, to set persist-credentials to
false while preserving the existing checkout configuration.
In `@README.markdown`:
- Line 3: Update the README description of LuneticsTimezoneBundle’s request
resolution to state that timezone resolution occurs only during the main
kernel.request and that Symfony subrequests reuse the main-request result;
remove the implication that each request resolves independently.
In `@src/Bridge/Form/TimezoneTypeExtension.php`:
- Around line 26-29: Update the view_timezone default in configureOptions so it
returns model_timezone when model_timezone differs from the provider timezone
and reference_date is null; otherwise preserve the existing provider-timezone
default. Add coverage for both TimeType and DateTimeType using differing
model/provider timezones without a reference_date.
In `@src/Bridge/Messenger/TimezoneStamp.php`:
- Around line 13-21: Update the TimezoneStamp constructor to assign the
canonical value returned by TimezoneId::fromString($timezone)->value() to the
readonly timezone property, matching the normalization performed by
__unserialize() while preserving validation and toTimezoneId() behavior.
In `@src/Context/CurrentTimezoneProvider.php`:
- Around line 30-34: Update getResolutionForRequest() to check
$this->context->current() before reading the supplied request’s
RESOLUTION_ATTRIBUTE, matching getResolution()’s precedence and preserving the
existing defaultResolution() fallback when neither source provides a
TimezoneResolution.
In `@src/Controller/BrowserTimezoneController.php`:
- Around line 61-78: Update the event dispatch in the controller method
containing storage->write and TimezonePreferenceChangedEvent so listener
exceptions cannot replace the already-successful response with a server error.
Isolate dispatcher->dispatch from the persistence try/catch using the project’s
established listener-failure handling approach, while preserving persistence
failure responses and returning the existing $response after dispatch.
In `@src/Resolver/StoredPreferenceTimezoneResolver.php`:
- Line 21: Update StoredPreferenceTimezoneResolver::READ_ATTRIBUTE and its cache
read/write logic so the key includes the identity of the configured storage
instance, preventing resolver instances from sharing cached reads across
different backends. Before changing the key format, inspect all READ_ATTRIBUTE
and preference_read consumers and update any dependent access consistently while
preserving the existing source-filter behavior.
In `@Tests/Bridge/Console/DebugTimezoneCommandTest.php`:
- Around line 20-25: Harden the ordering check in the DebugTimezoneCommand test
by first asserting that both “browser” and “manual” are present in $display,
then compare their positions with assertLessThan. Keep the existing output and
status assertions unchanged.
---
Nitpick comments:
In `@Resources/public/timezone.js`:
- Around line 1-25: Update syncBrowserTimezone to create an AbortController,
pass its signal to the fetch call, and abort the request after a defined timeout
so stalled requests settle; ensure the timeout is cleared when fetch completes
while preserving the existing success and failure handling.
In `@src/Bridge/MaxMind/GeoIp2CityReader.php`:
- Around line 19-28: Update GeoIp2CityReader::timezoneForIp to catch
\InvalidArgumentException from the reader->city() lookup and wrap it with
TimezoneResolverException::maxMindLookupFailed($exception), alongside the
existing InvalidDatabaseException and BadMethodCallException handling. Preserve
the current null result for AddressNotFoundException.
In `@src/Bridge/WebProfiler/TimezoneDataCollector.php`:
- Around line 50-128: Define a single private constant for the ten diagnostics
keys in their required order, then reuse it in collect(), reset(), and
isDiagnostics() instead of repeating the literal key list. Ensure array
construction and validation remain aligned so getDiagnostics() accepts data
whenever the same keys are present in the defined order.
In `@src/Controller/BrowserTimezoneController.php`:
- Around line 42-43: Update the status constant in the oversized-content branch
of BrowserTimezoneController so it uses the Symfony-defined
Response::HTTP_REQUEST_ENTITY_TOO_LARGE constant instead of the unavailable
payload-too-large constant, while preserving the 413 response behavior.
In `@src/LuneticsTimezoneBundle.php`:
- Around line 214-219: Remove the local resolver catalog construction in the
command registration flow, including the `$addToCatalog` closure, all calls to
it, and the `usort`/`array_map` transformations. Register `DebugTimezoneCommand`
with an empty catalog placeholder so `TimezoneCompilerPass` remains the sole
owner of resolver ordering and catalog population.
In `@src/Resolver/LocaleMappingTimezoneResolver.php`:
- Around line 47-50: Update the normalizeLocale method to normalize locale
casing in addition to replacing hyphens with underscores, ensuring configured
keys and request locales match case-insensitively and allowing the collision
check to detect case-only duplicates.
In `@src/Resolver/UserTimezoneResolver.php`:
- Around line 32-42: Update both user timezone accessor try/catch blocks in
UserTimezoneResolver to catch \Throwable instead of \Exception, preserving the
existing wrapping through
TimezoneResolverException::userAccessorFailed($exception) for errors from either
$user->getTimezone() or $this->accessor->getTimezoneForUser($user).
In `@src/Storage/CookieTimezoneStorage.php`:
- Around line 177-192: Extract the duplicated preference-envelope codec into a
shared helper used by both storage backends. In
src/Storage/CookieTimezoneStorage.php lines 177-192 and its read() validation at
lines 82-94, move parseAtom() and envelope key/type validation; in
src/Storage/SessionTimezoneStorage.php lines 71-102, replace the local encode(),
decode(), and parseAtom() implementations with the shared helper while
preserving the existing envelope format and validation behavior.
In `@Tests/Browser/timezone.test.mjs`:
- Around line 5-13: Capture the original globalThis.CustomEvent alongside the
existing originalFetch, originalIntl, and originalWindow values, then restore
globalThis.CustomEvent in afterEach after installBrowser() overwrites it.
In `@Tests/EventListener/PreferenceDiagnosticsFlagsTest.php`:
- Around line 388-434: Replace the anonymous storage class in storage() and
RecordingCleanupStorage with one shared configurable
TimezonePreferenceStorageInterface test double. Preserve absent reads, optional
write/clear failures, and clear-call recording so
testSubRequestDoesNotInspectOrCleanCopiedInvalidPreference and the
failure-strategy tests can configure and reuse the same implementation.
In `@Tests/Integration/BundleKernelSmokeTest.php`:
- Around line 44-53: Update BundleKernelSmokeTest tearDown to restore the
exception handler only when boot() recorded a successful Symfony handler
registration; track that registration explicitly during boot and clear the
tracking state after restoration. Remove the unconditional
restore_exception_handler() call while preserving kernel shutdown and
runtime-directory cleanup.
In `@Tests/Storage/CookieTimezoneStorageTest.php`:
- Around line 172-181: Remove the unused $clock parameter from encodedCookie(),
then update both call sites in testExpiredAndFutureDatedCookiesAreRejected() to
pass only the parameters the method uses: $storage and $recordedAt.
- Around line 109-136: Add a focused test for CookieTimezoneStorage::clear()
using one shared Response: call write() followed by clear() on that same
response, then assert the cookie retains the originally written value and
attributes, confirming the fresh-cookie anti-clobber behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9842e625-da80-4f39-9022-29f520a1a3db
📒 Files selected for processing (143)
.gitattributes.github/workflows/tests.yml.gitignore.travis.ymlCHANGELOG.mdDependencyInjection/Compiler/GuesserCompilerPass.phpDependencyInjection/Configuration.phpDependencyInjection/LuneticsTimezoneExtension.phpEvent/FilterTimezoneEvent.phpEventListener/TimezoneListener.phpException/TimezoneGuesserException.phpLICENSELuneticsTimezoneBundle.phpREADME.markdownResources/config/LocaleMapper.ymlResources/config/routes.phpResources/config/services.xmlResources/doc/guesser.mdResources/doc/index.mdResources/doc/installation.mdResources/doc/resolvers.mdResources/doc/scope.mdResources/doc/v2-implementation-plan.mdResources/public/timezone.jsResources/views/Collector/timezone.html.twigTests/Bridge/Console/DebugTimezoneCommandTest.phpTests/Bridge/Form/TimezoneTypeExtensionTest.phpTests/Bridge/MaxMind/GeoIp2CityReaderTest.phpTests/Bridge/MaxMind/MaxMindDatabaseCheckCommandTest.phpTests/Bridge/Messenger/TimezoneMessengerTest.phpTests/Bridge/Twig/TwigTimezoneScopeTest.phpTests/Bridge/Twig/TwigTimezoneSubscriberTest.phpTests/Bridge/WebProfiler/TimezoneDataCollectorTest.phpTests/Browser/timezone.test.mjsTests/Context/TimezoneExecutionContextTest.phpTests/Controller/BrowserTimezoneControllerTest.phpTests/DependencyInjection/LuneticsTimezoneBundleTest.phpTests/DependencyInjection/LuneticsTimezoneExtensionTest.phpTests/Distribution/ExportPolicyTest.phpTests/EventListener/PreferenceDiagnosticsFlagsTest.phpTests/EventListener/ResolveTimezoneListenerV2Test.phpTests/Integration/BundleKernelSmokeTest.phpTests/Resolution/TimezoneResolverChainTest.phpTests/Resolver/CallableTimezoneResolverTest.phpTests/Resolver/HeaderTimezoneResolverTest.phpTests/Resolver/LocaleMappingTimezoneResolverTest.phpTests/Resolver/LocaleTimezoneResolverTest.phpTests/Resolver/MaxMindTimezoneResolverTest.phpTests/Resolver/OidcTimezoneResolverTest.phpTests/Resolver/RequestAttributeTimezoneResolverTest.phpTests/Resolver/StoredPreferenceTimezoneResolverTest.phpTests/Resolver/UserTimezoneResolverTest.phpTests/Resources/BrowserRouteTest.phpTests/Storage/CookieTimezoneStorageTest.phpTests/Storage/StorageDomainTest.phpTests/Timezone/TimezoneIdTest.phpTests/TimezoneGuesser/GeoTimezoneGuesserTest.phpTests/TimezoneGuesser/LocaleTimezoneGuesserTest.phpTests/TimezoneGuesser/LocalemapperGuesserTest.phpTests/TimezoneGuesser/TimezoneGuesserManagerTest.phpTests/TimezoneProvider/TimezoneProviderTest.phpTests/Validator/TimezoneValidatorTest.phpTests/bootstrap.phpTimezoneBundleEvents.phpTimezoneGuesser/GeoTimezoneGuesser.phpTimezoneGuesser/LocaleTimezoneGuesser.phpTimezoneGuesser/LocalemapperTimezoneGuesser.phpTimezoneGuesser/TimezoneGuesserInterface.phpTimezoneGuesser/TimezoneGuesserManager.phpTimezoneProvider/TimezoneProvider.phpUPGRADE-2.0.mdValidator/Timezone.phpValidator/TimezoneValidator.phpcomposer.jsonpackage.jsonphpstan.neon.distphpunit.xml.distscripts/no-dev-smoke.phpsrc/Bridge/Console/DebugTimezoneCommand.phpsrc/Bridge/Form/TimezoneTypeExtension.phpsrc/Bridge/MaxMind/CallableMaxMindCityReader.phpsrc/Bridge/MaxMind/GeoIp2CityReader.phpsrc/Bridge/MaxMind/LazyGeoIp2CityReader.phpsrc/Bridge/MaxMind/MaxMindCityReaderInterface.phpsrc/Bridge/MaxMind/MaxMindDatabaseCheckCommand.phpsrc/Bridge/Messenger/DispatchTimezoneMiddleware.phpsrc/Bridge/Messenger/TimezoneStamp.phpsrc/Bridge/Messenger/WorkerTimezoneMiddleware.phpsrc/Bridge/Twig/TwigTimezoneScope.phpsrc/Bridge/Twig/TwigTimezoneSubscriber.phpsrc/Bridge/WebProfiler/TimezoneDataCollector.phpsrc/Clock/SystemClock.phpsrc/Context/CurrentTimezoneProvider.phpsrc/Context/CurrentTimezoneProviderInterface.phpsrc/Context/TimezoneExecutionContext.phpsrc/Context/TimezoneExecutionContextInterface.phpsrc/Contract/Oidc/OidcClaimsProviderInterface.phpsrc/Contract/User/TimezoneAwareUserInterface.phpsrc/Contract/User/UserTimezoneAccessorInterface.phpsrc/Controller/BrowserTimezoneController.phpsrc/DependencyInjection/Compiler/TimezoneCompilerPass.phpsrc/Event/TimezonePreferenceChangedEvent.phpsrc/Event/TimezoneResolvedEvent.phpsrc/EventListener/InvalidPreferenceCleanupListener.phpsrc/EventListener/ResolveTimezoneListener.phpsrc/Exception/InvalidTimezoneException.phpsrc/Exception/PersistenceFailureExceptionInterface.phpsrc/Exception/ResolutionFailureExceptionInterface.phpsrc/Exception/TimezoneException.phpsrc/Exception/TimezoneResolverException.phpsrc/Exception/TimezoneStorageException.phpsrc/LuneticsTimezoneBundle.phpsrc/Resolution/PersistenceFailureStrategy.phpsrc/Resolution/ResolutionAttemptOutcome.phpsrc/Resolution/ResolutionFailureStrategy.phpsrc/Resolution/ResolutionKind.phpsrc/Resolution/TimezoneResolution.phpsrc/Resolution/TimezoneResolutionAttempt.phpsrc/Resolution/TimezoneResolutionTrace.phpsrc/Resolution/TimezoneResolverChain.phpsrc/Resolver/CallableTimezoneResolver.phpsrc/Resolver/CountryTimezoneSourceInterface.phpsrc/Resolver/HeaderTimezoneResolver.phpsrc/Resolver/HeaderTrustMode.phpsrc/Resolver/LocaleMappingTimezoneResolver.phpsrc/Resolver/LocaleTimezoneResolver.phpsrc/Resolver/MaxMindTimezoneResolver.phpsrc/Resolver/OidcTimezoneResolver.phpsrc/Resolver/PhpCountryTimezoneSource.phpsrc/Resolver/RequestAttributeTimezoneResolver.phpsrc/Resolver/StoredPreferenceTimezoneResolver.phpsrc/Resolver/TimezoneResolverInterface.phpsrc/Resolver/UserTimezoneResolver.phpsrc/Storage/CookieTimezoneStorage.phpsrc/Storage/PreferenceReadStatus.phpsrc/Storage/PreferenceSource.phpsrc/Storage/PreferenceWriteMarkingStorage.phpsrc/Storage/ResponseScopedStorageInterface.phpsrc/Storage/SessionTimezoneStorage.phpsrc/Storage/TimezonePreference.phpsrc/Storage/TimezonePreferenceRead.phpsrc/Storage/TimezonePreferenceStorageInterface.phpsrc/Timezone/TimezoneId.php
💤 Files with no reviewable changes (27)
- Exception/TimezoneGuesserException.php
- Event/FilterTimezoneEvent.php
- Tests/DependencyInjection/LuneticsTimezoneExtensionTest.php
- Resources/doc/guesser.md
- .travis.yml
- TimezoneGuesser/GeoTimezoneGuesser.php
- DependencyInjection/LuneticsTimezoneExtension.php
- TimezoneGuesser/TimezoneGuesserInterface.php
- DependencyInjection/Configuration.php
- TimezoneBundleEvents.php
- Tests/Validator/TimezoneValidatorTest.php
- LuneticsTimezoneBundle.php
- Validator/TimezoneValidator.php
- Resources/config/LocaleMapper.yml
- Tests/TimezoneGuesser/LocaleTimezoneGuesserTest.php
- EventListener/TimezoneListener.php
- Tests/TimezoneGuesser/GeoTimezoneGuesserTest.php
- DependencyInjection/Compiler/GuesserCompilerPass.php
- TimezoneProvider/TimezoneProvider.php
- Tests/TimezoneGuesser/LocalemapperGuesserTest.php
- Tests/TimezoneGuesser/TimezoneGuesserManagerTest.php
- TimezoneGuesser/LocaleTimezoneGuesser.php
- Resources/config/services.xml
- Validator/Timezone.php
- Tests/TimezoneProvider/TimezoneProviderTest.php
- TimezoneGuesser/TimezoneGuesserManager.php
- TimezoneGuesser/LocalemapperTimezoneGuesser.php
Comment on lines
+13
to
+21
| public function __construct(public readonly string $timezone) | ||
| { | ||
| TimezoneId::fromString($timezone); | ||
| } | ||
|
|
||
| public function toTimezoneId(): TimezoneId | ||
| { | ||
| return TimezoneId::fromString($this->timezone); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize $timezone in the constructor to match __unserialize().
The constructor validates the identifier but keeps the raw input string. __unserialize() normalizes to $validated->value(). If the caller passes a non-canonical alias, a freshly constructed TimezoneStamp exposes a different $timezone string than the same stamp after a serialize/unserialize round trip. Store the canonical value at construction time.
🔧 Proposed fix
final class TimezoneStamp implements StampInterface
{
- public function __construct(public readonly string $timezone)
+ public readonly string $timezone;
+
+ public function __construct(string $timezone)
{
- TimezoneId::fromString($timezone);
+ $this->timezone = TimezoneId::fromString($timezone)->value();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function __construct(public readonly string $timezone) | |
| { | |
| TimezoneId::fromString($timezone); | |
| } | |
| public function toTimezoneId(): TimezoneId | |
| { | |
| return TimezoneId::fromString($this->timezone); | |
| } | |
| public readonly string $timezone; | |
| public function __construct(string $timezone) | |
| { | |
| $this->timezone = TimezoneId::fromString($timezone)->value(); | |
| } | |
| public function toTimezoneId(): TimezoneId | |
| { | |
| return TimezoneId::fromString($this->timezone); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bridge/Messenger/TimezoneStamp.php` around lines 13 - 21, Update the
TimezoneStamp constructor to assign the canonical value returned by
TimezoneId::fromString($timezone)->value() to the readonly timezone property,
matching the normalization performed by __unserialize() while preserving
validation and toTimezoneId() behavior.
Comment on lines
+30
to
+34
| public function getResolutionForRequest(Request $request): TimezoneResolution | ||
| { | ||
| $resolution = $request->attributes->get(self::RESOLUTION_ATTRIBUTE); | ||
| return $resolution instanceof TimezoneResolution ? $resolution : $this->defaultResolution(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the execution context in getResolutionForRequest for consistency.
getResolution() checks $this->context->current() first, then falls back to the main request's resolution, then to the default. getResolutionForRequest() only inspects the given $request's attributes and falls back straight to the default. It never considers an active execution context.
Any caller that invokes getResolutionForRequest($request) directly while inside a TimezoneExecutionContextInterface::run() scope (for example, from a bridge that renders a specific request) gets a result inconsistent with getResolution(). Because Symfony sub-requests are new Request objects that do not automatically inherit the main request's attributes, and ResolveTimezoneListener only sets RESOLUTION_ATTRIBUTE for main requests (per the listener snippet), calling getResolutionForRequest($subRequest) silently falls back to the default timezone instead of the resolved one.
🔧 Proposed fix to check context before falling back to request attributes
public function getResolutionForRequest(Request $request): TimezoneResolution
{
+ if (null !== $timezone = $this->context->current()) {
+ return new TimezoneResolution($timezone, 'execution_context', ResolutionKind::EXPLICIT);
+ }
$resolution = $request->attributes->get(self::RESOLUTION_ATTRIBUTE);
return $resolution instanceof TimezoneResolution ? $resolution : $this->defaultResolution();
}Confirm whether callers ever pass sub-requests to getResolutionForRequest, since that changes how urgent this gap is.
#!/bin/bash
# Description: Find callers of getResolutionForRequest to check whether sub-requests are ever passed.
rg -n -C5 'getResolutionForRequest\(' --type=php🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Context/CurrentTimezoneProvider.php` around lines 30 - 34, Update
getResolutionForRequest() to check $this->context->current() before reading the
supplied request’s RESOLUTION_ATTRIBUTE, matching getResolution()’s precedence
and preserving the existing defaultResolution() fallback when neither source
provides a TimezoneResolution.
Comment on lines
+61
to
+78
| $response = new Response('', Response::HTTP_NO_CONTENT); | ||
| try { | ||
| $read = $this->storage->read($request); | ||
| $previous = PreferenceReadStatus::VALID === $read->status ? $read->preference : null; | ||
| if (PreferenceSource::MANUAL === $previous?->source) { | ||
| return $response; | ||
| } | ||
| $current = new TimezonePreference($timezone, PreferenceSource::BROWSER, \DateTimeImmutable::createFromInterface($this->clock->now())); | ||
| if (null !== $previous && $previous->equals($current)) { | ||
| return $response; | ||
| } | ||
| $this->storage->write($request, $response, $current); | ||
| } catch (PersistenceFailureExceptionInterface) { | ||
| return new Response('', Response::HTTP_SERVICE_UNAVAILABLE); | ||
| } | ||
| $this->dispatcher->dispatch(new TimezonePreferenceChangedEvent($request, $previous, $current)); | ||
| return $response; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard the event dispatch so a listener failure cannot discard a completed write.
The event dispatch at Line 76 sits outside the try/catch that guards persistence at Lines 62-75. $response already carries the effect of $this->storage->write(...) (for CookieTimezoneStorage, the write is only realized through the Set-Cookie header on this same $response). If a TimezonePreferenceChangedEvent listener throws, Symfony discards $response and returns a 500 error instead. The client never receives the cookie, even though the write already succeeded internally. Session-based storage would already be durable at this point, so the two storage backends behave inconsistently on listener failure.
Isolate the dispatch so a listener exception cannot revert the successful response.
🛠️ Proposed fix to isolate listener failures from the response
$this->storage->write($request, $response, $current);
} catch (PersistenceFailureExceptionInterface) {
return new Response('', Response::HTTP_SERVICE_UNAVAILABLE);
}
- $this->dispatcher->dispatch(new TimezonePreferenceChangedEvent($request, $previous, $current));
+ try {
+ $this->dispatcher->dispatch(new TimezonePreferenceChangedEvent($request, $previous, $current));
+ } catch (\Throwable) {
+ // The preference write already succeeded; do not let a listener failure
+ // discard the response (and, for cookie storage, the already-set cookie).
+ }
return $response;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $response = new Response('', Response::HTTP_NO_CONTENT); | |
| try { | |
| $read = $this->storage->read($request); | |
| $previous = PreferenceReadStatus::VALID === $read->status ? $read->preference : null; | |
| if (PreferenceSource::MANUAL === $previous?->source) { | |
| return $response; | |
| } | |
| $current = new TimezonePreference($timezone, PreferenceSource::BROWSER, \DateTimeImmutable::createFromInterface($this->clock->now())); | |
| if (null !== $previous && $previous->equals($current)) { | |
| return $response; | |
| } | |
| $this->storage->write($request, $response, $current); | |
| } catch (PersistenceFailureExceptionInterface) { | |
| return new Response('', Response::HTTP_SERVICE_UNAVAILABLE); | |
| } | |
| $this->dispatcher->dispatch(new TimezonePreferenceChangedEvent($request, $previous, $current)); | |
| return $response; | |
| } | |
| $response = new Response('', Response::HTTP_NO_CONTENT); | |
| try { | |
| $read = $this->storage->read($request); | |
| $previous = PreferenceReadStatus::VALID === $read->status ? $read->preference : null; | |
| if (PreferenceSource::MANUAL === $previous?->source) { | |
| return $response; | |
| } | |
| $current = new TimezonePreference($timezone, PreferenceSource::BROWSER, \DateTimeImmutable::createFromInterface($this->clock->now())); | |
| if (null !== $previous && $previous->equals($current)) { | |
| return $response; | |
| } | |
| $this->storage->write($request, $response, $current); | |
| } catch (PersistenceFailureExceptionInterface) { | |
| return new Response('', Response::HTTP_SERVICE_UNAVAILABLE); | |
| } | |
| try { | |
| $this->dispatcher->dispatch(new TimezonePreferenceChangedEvent($request, $previous, $current)); | |
| } catch (\Throwable) { | |
| // The preference write already succeeded; do not let a listener failure | |
| // discard the response (and, for cookie storage, the already-set cookie). | |
| } | |
| return $response; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Controller/BrowserTimezoneController.php` around lines 61 - 78, Update
the event dispatch in the controller method containing storage->write and
TimezonePreferenceChangedEvent so listener exceptions cannot replace the
already-successful response with a server error. Isolate dispatcher->dispatch
from the persistence try/catch using the project’s established listener-failure
handling approach, while preserving persistence failure responses and returning
the existing $response after dispatch.
| { | ||
| public const MANUAL_PRIORITY = 925; | ||
| public const BROWSER_PRIORITY = 800; | ||
| public const READ_ATTRIBUTE = '_lunetics_timezone.preference_read'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope the read cache to the storage instance.
READ_ATTRIBUTE is one fixed key for all resolver instances. If the container registers two instances with different storage services, the first instance caches its read and the second instance consumes that result instead of reading its own backend. The source filter at line 47 then evaluates the wrong preference.
Include the storage identity in the cache key.
🐛 Proposed fix
- $cached = $request->attributes->get(self::READ_ATTRIBUTE);
+ $cacheKey = self::READ_ATTRIBUTE.'.'.spl_object_id($this->storage);
+ $cached = $request->attributes->get($cacheKey);
if ($cached instanceof TimezonePreferenceRead) {
$read = $cached;
} else {
try {
$read = $this->storage->read($request);
} catch (PersistenceFailureExceptionInterface $failure) {
if (PersistenceFailureStrategy::THROW === $this->failureStrategy) {
throw $failure;
}
return null;
}
- $request->attributes->set(self::READ_ATTRIBUTE, $read);
+ $request->attributes->set($cacheKey, $read);
}Note that other code may read READ_ATTRIBUTE directly, so confirm the consumers before you change the key format.
#!/bin/bash
# Description: Find resolver registrations and READ_ATTRIBUTE consumers.
rg -nP -C 6 'StoredPreferenceTimezoneResolver' --glob '*.php'
rg -nP -C 3 'READ_ATTRIBUTE|preference_read' --glob '*.php'Also applies to: 33-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Resolver/StoredPreferenceTimezoneResolver.php` at line 21, Update
StoredPreferenceTimezoneResolver::READ_ATTRIBUTE and its cache read/write logic
so the key includes the identity of the configured storage instance, preventing
resolver instances from sharing cached reads across different backends. Before
changing the key format, inspect all READ_ATTRIBUTE and preference_read
consumers and update any dependent access consistently while preserving the
existing source-filter behavior.
The view_timezone default always returns the current timezone, but Symfony's TimeType refuses a view timezone that differs from an explicitly configured model_timezone unless a reference_date resolves the offset — enabling the opt-in form bridge then makes such a form unbuildable. A field declared as TimeType with model_timezone: UTC and no reference_date throws 'Using different values for the "model_timezone" and "view_timezone" options without configuring a reference date is not supported.' as soon as the bridge is active. DateType and DateTimeType resolve the offset from the date itself and were never affected. The default now yields to an explicitly configured model timezone when no reference date is present, so the bridge steps aside instead of breaking the form, and still applies the current timezone whenever the conversion is defined. Tests cover all three extended types with and without a reference date. The reference-date probe reads the option value rather than using isset(), which reports true for a defined-but-null option on Options. Reported by the repository's coderabbit bot.
actions/checkout writes the job's GitHub token into the local git config by default, so every later step of the same job — composer update, npm test, the smoke script — runs third-party code that can read it from disk. Any compromised or malicious dependency executed during a workflow run could exfiltrate a token with write access to this repository. All four checkout steps now set persist-credentials: false; no step in these jobs pushes or otherwise needs the credential. Flagged by zizmor via the repository's coderabbit bot.
strpos() returns false when a needle is absent, and comparing false against an integer position juggles it to 0, so the ordering assertion passes whenever 'manual' is missing but 'browser' is printed. A regression that drops the manual resolver from the debug output would keep this test green while the command silently lost a row. Both positions are asserted to be integers first, so absence fails loudly, and only then is their order compared. Reported by the repository's coderabbit bot.
The README opens with 'resolves an IANA timezone for each Symfony request', which reads as if every subrequest ran the resolver chain, while the bundle resolves only on the main kernel.request and subrequests reuse that resolution. Readers can build integrations around a per-subrequest resolution that the bundle never performs. The sentence now names the main request and the subrequest reuse explicitly. Reported by the repository's coderabbit bot.
Two seams are only safe under constraints that neither the code nor the documentation states: the per-request storage read is cached under one storage-agnostic key, and a throwing listener of the preference-changed event discards the response that carries a cookie write. An application registering a second StoredPreferenceTimezoneResolver against another storage would silently consume the configured storage's cached read, and a failing event listener loses a cookie write while an equivalent session write is already durable. Both constraints are now written down — on the constant itself and in the custom-storage section — including the recommendation to keep such listeners failure-free or defer them to kernel.terminate. The behaviour is unchanged: swallowing listener exceptions would hide application errors, and rekeying the cache would touch three consumers to guard a configuration the documentation does not describe.
The declared range admits versions that are dying or already dead: Symfony 8.0 left support in July 2026, Symfony 6.4 stops receiving bug fixes in November 2026, and PHP 8.2 reaches end of life in December 2026 — a 2.0 released now would ship with a floor that outlives the release by months. The floor also permits symfony/http-foundation releases affected by CVE-2026-48736, whose IpUtils::PRIVATE_SUBNETS omits the 6to4 and NAT64 transition prefixes. Consumers installing at the floor receive an unpatched http-foundation, and CI spends two of its seven matrix legs on an unmaintained Symfony 8.0 and an LTS that V2 adopters cannot migrate to anyway. The package now requires PHP ^8.3 and Symfony ^7.4.13 || ^8.1, tracking the current LTS and the current stable while ^8.1 keeps later Symfony 8 minors admissible. The matrix drops to four legs (8.3/8.4 with 7.4, 8.4/8.5 with 8.1), the prefer-lowest job gains a production-dependency audit so the floor cannot silently rot, and README, installation, plan and changelog state the policy with its dates. Applications on Symfony 6.4 stay on the 1.x line — V2 breaks from it without a compatibility layer regardless, so the supported range is not what keeps them from upgrading. Verified by resolution: prefer-lowest locks http-foundation v7.4.13, highest locks v8.1.2.
lunetics
changed the title
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Resources/doc/installation.md (1)
5-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the PHP/Symfony version pairing.
This line states the requirement as
PHP ^8.3andSymfony ^7.4.13 || ^8.1together, without noting that each Symfony line has its own PHP floor. Symfony 8.1 requires PHP 8.4 or higher. A reader running PHP 8.3 who pinssymfony/*: ^8.1hits a Composer platform-requirement failure that this line does not anticipate.The CI matrix in
v2-implementation-plan.md(PHP 8.3/8.4 with Symfony 7.4, PHP 8.4/8.5 with Symfony 8.1) already states the correct pairing. Mirror that pairing here.📝 Proposed clarification
-Install PHP `^8.3` and Symfony `^7.4.13 || ^8.1`, then: +Install PHP `^8.3` with Symfony `^7.4.13`, or PHP `^8.4` with Symfony `^8.1`, then:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Resources/doc/installation.md` around lines 5 - 13, Update the installation requirements text to state the compatible PHP/Symfony pairings: PHP 8.3 or 8.4 with Symfony 7.4.13, and PHP 8.4 or 8.5 with Symfony 8.1. Keep the existing installation command and follow-up setup guidance unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Resources/doc/installation.md`:
- Around line 5-13: Update the installation requirements text to state the
compatible PHP/Symfony pairings: PHP 8.3 or 8.4 with Symfony 7.4.13, and PHP 8.4
or 8.5 with Symfony 8.1. Keep the existing installation command and follow-up
setup guidance unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15b39703-5b74-4bc7-8114-56b85bba3c7a
📒 Files selected for processing (6)
.github/workflows/tests.ymlCHANGELOG.mdREADME.markdownResources/doc/installation.mdResources/doc/v2-implementation-plan.mdcomposer.json
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- .github/workflows/tests.yml
- README.markdown
The size guard returns the bare literal 413 while every other status in the controller uses a Response constant, and the obvious candidate name HTTP_PAYLOAD_TOO_LARGE does not exist in the supported Symfony range — a later cleanup would reach for it and break the build. Nobody reading the controller can tell whether 413 was deliberate or a placeholder. The guard now returns Response::HTTP_REQUEST_ENTITY_TOO_LARGE, the name Symfony actually defines (verified in the installed 7.4.14). Reported by the repository's coderabbit bot.
tearDown() calls restore_exception_handler() unconditionally, but only a booted debug kernel installs Symfony's handler. When boot() fails earlier, the call pops PHPUnit's own handler instead. A kernel that fails to boot therefore leaves the suite without PHPUnit's exception handler, so errors of later tests are swallowed and the run can end green on a broken build. The teardown now restores only after a boot that actually completed. Reported by the repository's coderabbit bot.
The rule that clear() must not remove a preference written to the same response is only covered end to end through the cleanup listener, and the encodedCookie() helper carries a clock parameter it never uses. A regression in that guard would surface as a confusing listener-level failure, or stay unnoticed if the listener path changes. Two focused tests now cover both branches directly — a fresh write survives, a stale cookie without a fresh write is removed — and the dead parameter is gone. Reported by the repository's coderabbit bot.
The requirements read as if PHP ^8.3 combined freely with the whole Symfony range, but Symfony 8 requires PHP 8.4 — a reader on PHP 8.3 who pins ^8.1 hits a platform-requirement failure the text does not anticipate. The README and installation guide now name both pairings explicitly. Reported by the repository's coderabbit bot.
The declared dev floor admits phpstan 2.1.0 with phpstan-symfony 2.0.0, which cannot follow the Config component's fluent tree builder — the prefer-lowest CI job fails with 247 errors while every other leg passes, because a local install resolves far newer analyzers. Any contributor running composer update --prefer-lowest, and the CI job guarding the dependency floor, hit an analysis failure that has nothing to do with the code under test. The floors are now phpstan ^2.2 and phpstan-symfony ^2.0.20, the lowest pair that analyzes this code cleanly — verified by resolving exactly 2.2.0 and 2.0.20 in a prefer-lowest install and running composer check there (PHPStan clean, 204 tests).
Owner
Author
|
@greptile-apps review Full review requested for the complete branch (the automatic run stopped at the 100-file limit). Scope of note since the last greptile pass: storage-decoration wiring, response-scoped preference storage, MaxMind range list, Form bridge view_timezone default, distribution-archive gate, and the supported-version change (PHP ^8.3, Symfony ^7.4.13 || ^8.1). |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/Integration/BundleKernelSmokeTest.php (1)
45-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore both PHP error and exception handlers in
tearDown().Symfony's
ErrorHandler::register()installs both handlers: a PHP error handler viaset_error_handler()and a PHP exception handler viaset_exception_handler(). When the kernel boots withdebug=true, FrameworkBundle callsErrorHandler::register(). Both tests in this file boot withdebug=true(lines 65 and 169).The current
tearDown()implementation callsrestore_exception_handler()when$this->bootedDebugKernelis true, but omitsrestore_error_handler(). The installed error handler remains active after teardown. This leaks handler state into subsequent tests in the PHPUnit process, potentially changing how PHP warnings, notices, or deprecations are handled, causing flaky or misleading test failures unrelated to the actual test code.Add
restore_error_handler()alongsiderestore_exception_handler():Proposed fix
if ($this->bootedDebugKernel) { restore_exception_handler(); + restore_error_handler(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Integration/BundleKernelSmokeTest.php` around lines 45 - 60, Update tearDown() in the bootedDebugKernel cleanup branch to call restore_error_handler() alongside restore_exception_handler(), ensuring both handlers installed during debug kernel boot are restored before resetting the flag and continuing cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Tests/Integration/BundleKernelSmokeTest.php`:
- Around line 45-60: Update tearDown() in the bootedDebugKernel cleanup branch
to call restore_error_handler() alongside restore_exception_handler(), ensuring
both handlers installed during debug kernel boot are restored before resetting
the flag and continuing cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f4f2dc91-f6fe-4b41-8bba-8c9319785335
📒 Files selected for processing (6)
README.markdownResources/doc/installation.mdTests/Integration/BundleKernelSmokeTest.phpTests/Storage/CookieTimezoneStorageTest.phpcomposer.jsonsrc/Controller/BrowserTimezoneController.php
🚧 Files skipped from review as they are similar to previous changes (3)
- composer.json
- src/Controller/BrowserTimezoneController.php
- Resources/doc/installation.md
Symfony's ErrorHandler::register() pushes a PHP error handler next to the exception handler, and a debug kernel boot triggers it, but the teardown pops only the exception handler. The error handler therefore stays active for the rest of the PHPUnit process, so warnings, notices and deprecations of later tests run through Symfony's handler instead of PHPUnit's — turning unrelated tests flaky or hiding their diagnostics. Both handlers are now restored behind the same booted-kernel guard. Measured before the change: after shutdown and restore_exception_handler() the process still carried ErrorHandler::handleError, and only the added restore_error_handler() returned the handler state to its pre-boot value. Reported by the repository's coderabbit bot.
This reverts commit beff109 and keeps the surrounding boot guard. Restoring the error handler is correct in a bare PHP process — a booted debug kernel does leave Symfony's ErrorHandler installed there — but not inside PHPUnit, and that is the only place this teardown runs. Measured in the running suite: at teardown the active error handler is already PHPUnit\Runner\ErrorHandler, so the extra restore_error_handler() pops PHPUnit's own handler and leaves NULL behind. The runner detects exactly that and marked both smoke tests 'Test code or tested code removed error handlers other than its own'. The teardown restores the exception handler only, and the comment now records why the sibling call must not be added back.
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.
Why: V1 keeps per-request state in shared guesser services and persists every effective timezone — including locale guesses and the configured default — into the session, which freezes a weak first-request guess and is unsafe in long-running workers. V2 is a clean major that rebuilds resolution, persistence and framework integration around immutable, request-local contracts.
Changes:
TimezoneIdvalidation.date_default_timezone_set(), keeping Messenger workers and consecutive requests isolated.Hardening from the review rounds (commits
74d8f65..ebb3c9f):IpUtils::PRIVATE_SUBNETS: that constant omits the 6to4 and NAT64 transition prefixes below 6.4.41 / 7.4.13 / 8.0.13 (CVE-2026-48736), which would leak translated private addresses to the reader on the supported floor.__Secure-cookie prefix is now validated like its stricter__Host-sibling, instead of silently emitting a cookie browsers reject.git archiveoutput (PAX and GNU long names, symlinks, asserted exit code) instead of asserting.gitattributesagainst a copy of itself.failOnDeprecationactually binds again:ignoreDirectDeprecationshad filtered exactly the deprecation class this bundle can produce.Accepted risk: the profiler reports
preference_clearedfor a forwarded-controller write where the storage turned the cleanup into a no-op — diagnostics only, the preference itself survives. External review coverage is single-model: the grok arm failed four times (cancelled runs, one fabricated finding set), so omission coverage rests on codex alone.BC note: breaking by design for this published package. All 1.x guesser, provider, event and validator APIs are removed without a compatibility layer, and storage, user and OIDC contracts are new. The supported range is now PHP
^8.3and Symfony^7.4.13 || ^8.1: unmaintained Symfony 8.0 (support ended July 2026), the older 6.4 LTS (bug fixes end November 2026) and end-of-life PHP 8.2 (December 2026) are out, and the floor excludes the http-foundation versions affected by CVE-2026-48736. Applications on 6.4 stay on the 1.x line. Migration path: UPGRADE-2.0.md.Verification:
ebb3c9f.--prefer-lowestreproduced in a clean clone: resolvessymfony/http-foundation v6.4.0(12PRIVATE_SUBNETSentries, no2001::/32, no64:ff9b::/96), suite green 196/821 — this leg was deterministically red before the MaxMind fix.git archive HEAD | tar -ttop level equals the test's allowlist, and every tracked entry is either export-ignored or allowed (checked per entry withgit check-attr).References: rebuild
fe348df, fixes74d8f65..ebb3c9f· CVE-2026-48736 (symfony/http-foundation) · review artifacts under.review/codex-v2-rebuild-2026-07-27/(local, not committed).Greptile Summary
This PR is a full V2 rebuild of the
lunetics/timezone-bundle, replacing all legacy guesser/provider/event/validator APIs with a typed resolver chain, signed-cookie and session storage, PSR clock integration, and optional bridges for Twig, Messenger, MaxMind, OIDC, and the Symfony profiler. The new code is insrc/under PHP 8.2+ with strict types throughout.TimezoneResolverChaindrives pluggable resolvers (request attribute, header, user, OIDC, MaxMind, locale-mapping, locale-country) with per-attempt tracing and configurablecontinue/throwfailure strategies.SessionTimezoneStorageandCookieTimezoneStorage(HMAC-SHA256 signed, base64url encoded) implement a sharedTimezonePreferenceStorageInterface; the cookie storage validates__Host-/SameSite=Nonerequirements at construction and run time.BrowserTimezoneControllerreceives JSON POST with CSRF validation, reads/compares current preference, writes BROWSER-source preference, and dispatchesTimezonePreferenceChangedEvent.Confidence Score: 4/5
Safe to merge; the new architecture is well-structured, security-sensitive paths (HMAC signature verification, CSRF, SameSite/Host prefix guards) are implemented correctly, and the resolver chain handles failures and defaults cleanly.
The only non-trivial issue is that TimezoneStorageException thrown by CookieTimezoneStorage::resolveSecure() gets caught and re-wrapped by the RuntimeException catch in write(), causing the informative root-cause message to be hidden behind a generic one. The hardcoded priority literals in loadExtension() are a maintenance risk given the constants defined in StoredPreferenceTimezoneResolver. Neither affects current correctness; both are easy to fix.
src/Storage/CookieTimezoneStorage.php (exception re-wrapping in write()) and src/LuneticsTimezoneBundle.php (priority literals vs. constants).
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Client participant K as Symfony Kernel participant RTL as ResolveTimezoneListener participant TRC as TimezoneResolverChain participant R as Resolvers participant S as Storage participant BTC as BrowserTimezoneController participant IPCL as InvalidPreferenceCleanupListener C->>K: Any HTTP Request K->>RTL: kernel.request priority 1 RTL->>TRC: resolve(request) TRC->>R: iterate resolvers by priority R->>S: read preference S-->>R: TimezonePreferenceRead R-->>TRC: TimezoneResolution or null TRC-->>RTL: TimezoneResolution or default RTL->>K: set RESOLUTION_ATTRIBUTE RTL->>K: dispatch TimezoneResolvedEvent C->>BTC: POST /timezone BTC->>S: read preference S-->>BTC: TimezonePreferenceRead Note over BTC: Skip if MANUAL source BTC->>S: write BROWSER preference BTC->>K: dispatch TimezonePreferenceChangedEvent K->>IPCL: kernel.response IPCL->>S: clear if INVALID or EXPIREDPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: keep Symfony 8.1 and PHP 8.5 CI str..." | Re-trigger Greptile
Summary by CodeRabbit