-
-
Notifications
You must be signed in to change notification settings - Fork 8
Use browser timezone #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace App\Livewire\Localization; | ||
|
|
||
| use App\Traits\SessionTimezoneTrait; | ||
| use Illuminate\Contracts\View\View; | ||
| use Livewire\Component; | ||
|
|
||
| final class SwitchTimezoneComponent extends Component | ||
| { | ||
| use SessionTimezoneTrait; | ||
|
|
||
| public function render(): View | ||
| { | ||
| return view('livewire.localization.switch-timezone-component'); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace App\Traits; | ||
|
|
||
| use Carbon\CarbonTimezone; | ||
| use Carbon\Exceptions\InvalidTimeZoneException; | ||
| use Livewire\Attributes\Locked; | ||
| use Livewire\Attributes\Session; | ||
|
|
||
| trait SessionTimezoneTrait | ||
| { | ||
| use LoggingTrait; | ||
|
|
||
| #[Session(key: 'displayTimezone')] | ||
| #[Locked] | ||
| public ?string $displayTimezone = null; | ||
|
|
||
| public function getDisplayTimezone(): string | ||
| { | ||
| // For some reason, calling session('displayTimezone') will sometimes return a value | ||
| // when $this->displayTimezone is null. | ||
| return $this->displayTimezone ?? session('displayTimezone') ?? config('app.timezone'); | ||
| } | ||
|
|
||
| public function setDisplayTimezone(string $timezone): void | ||
| { | ||
| try { | ||
| // Try to create a CarbonTimezone object to validate the timezone string. | ||
| CarbonTimezone::create($timezone); | ||
|
|
||
| // If we get here, the timezone is valid, so we can set it. | ||
| $this->displayTimezone = $timezone; | ||
| } catch (InvalidTimeZoneException $e) { | ||
| $this->logError('Invalid timezone: ' . $timezone); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,11 @@ | ||
| <time datetime="{{ $comment->created_at->format('Y-m-d H:i:d') }}"> | ||
| <a href="#{{ $comment->id }}" class="footer-info" title="{{ trans('Permanent link to this comment') }}"> | ||
| <x-icons.icon-component filename="clock" class="icon-small" /> | ||
| {{ $comment->created_at->format('g:i a') }} | ||
| {{ $comment->created_at->tz($displayTimezone)->format('g:i a') }} | ||
| </a> | ||
|
|
||
| <span class="footer-info"> | ||
| <x-icons.icon-component filename="calendar3" class="icon-small" /> | ||
| {{ $comment->created_at->format('F j') }} | ||
| {{ $comment->created_at->tz($displayTimezone)->format('F j') }} | ||
| </span> | ||
| </time> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <div style="display: none;" wire:init="$js.setDisplayTimezone"></div> | ||
|
|
||
| @script | ||
| <script> | ||
| $js('setDisplayTimezone', () => { | ||
| const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any concerns with potential IANA zone mismatches between what a browser will supply and what PHP understands? there's some MDN discussion about non-canonicalization of TZ names, also, from an infra perspective, this will probably require keeping TZDB up-to-date on the relevant App & SQL instances. Probably not a real problem, but just calling out to avoid missing these updates then getting unknown timezones.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be ok as long as we're tolerant of unknown timezones server-side and let people override from a server-supplied list in preferences. For the majority of people just browsing casually the idea was to not have times show up in a weird timezone. |
||
| $wire.setDisplayTimezone(timezone); | ||
| }); | ||
| </script> | ||
| @endscript | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there is some input validation on this, but using this raw instead of parameterized feels like a potential SQL injection vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm expecting
DB::connection()->getPdo()->quoteto do the appropriate escaping to avoid SQL injection. It's unfortunately quite awkward to add bind parameters to queries constructed by the query builder as it only uses?placeholders, not named ones, so then you have to figure out the right ordering of bind parameters in a way that respects the rest of the query as constructed by Eloquent. I spent a while trying to do that and gave up, hence this attempt to manage interpolating a properly escaped value 😓