feat!: rebuild timezone handling for PHP 8.2 and Symfony 6.4–8.1#21
Draft
lunetics wants to merge 3 commits into
Draft
feat!: rebuild timezone handling for PHP 8.2 and Symfony 6.4–8.1#21lunetics wants to merge 3 commits into
lunetics wants to merge 3 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.
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.
Summary
src/and introduce typed resolver, storage, context, event, and exception contractsReview hardening
Validation
composer update --no-devplus package smoke and auditcomposer update --prefer-lowestplus PHPStan and 179 testsgit archivepolicy checksBreaking changes
V2 intentionally removes all legacy guesser, provider, event, and validator APIs without a compatibility layer. See UPGRADE-2.0.md.
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