Skip to content

Comments

Outputs time respecting backend preferences#572

Merged
LukeTowers merged 2 commits intowintercms:developfrom
arvislacis:backend-timezone-event-log
Jun 26, 2022
Merged

Outputs time respecting backend preferences#572
LukeTowers merged 2 commits intowintercms:developfrom
arvislacis:backend-timezone-event-log

Conversation

@arvislacis
Copy link
Contributor

Fixes #535

In addition it also fixes the same problem in the theme log detail page.

@mjauvin
Copy link
Member

mjauvin commented Jun 10, 2022

Would it make more sense to actually set the timezone on the Model dates from within the Storm library?

i.e. Database/Model.php (asDateTime() method)

@mjauvin
Copy link
Member

mjauvin commented Jun 10, 2022

I just tested adding backend timezone in Winter\Storm\Database\Model::asDateTime() and it works great.

@mjauvin
Copy link
Member

mjauvin commented Jun 10, 2022

diff --git a/src/Database/Model.php b/src/Database/Model.php
index 9f2666a..8d57d76 100644
--- a/src/Database/Model.php
+++ b/src/Database/Model.php
@@ -1,6 +1,7 @@
 <?php namespace Winter\Storm\Database;
         
 use Closure;
+use Config;
 use Winter\Storm\Support\Arr;
 use Winter\Storm\Support\Str;
 use Winter\Storm\Argon\Argon;
@@ -544,11 +545,13 @@ public function freshTimestamp()
      */
     protected function asDateTime($value)
     {
+        $tz = Config::get('cms.backendTimezone', 'UTC');
+
         // If this value is already a Argon instance, we shall just return it as is.
         // This prevents us having to re-instantiate a Argon instance when we know
         // it already is one, which wouldn't be fulfilled by the DateTime check.
         if ($value instanceof Argon) {
-            return $value;
+            return $value->tz($tz);
         }
 
         // If the value is already a DateTime instance, we will just skip the rest of
@@ -557,7 +560,7 @@ protected function asDateTime($value)
         if ($value instanceof DateTimeInterface) {
             return new Argon(
                 $value->format('Y-m-d H:i:s.u'),
-                $value->getTimezone()
+                $value->getTimezone() ?: $tz
             );
         }
         
@@ -565,14 +568,14 @@ protected function asDateTime($value)
         // and format a Carbon object from this timestamp. This allows flexibility
         // when defining your date fields as they might be UNIX timestamps here.
         if (is_numeric($value)) {
-            return Argon::createFromTimestamp($value);
+            return Argon::createFromTimestamp($value)->tz($tz);
         }   
         
         // If the value is in simply year, month, day format, we will instantiate the
         // Carbon instances from that format. Again, this provides for simple date
         // fields on the database, while still supporting Carbonized conversion.
         if ($this->isStandardDateFormat($value)) {
-            return Argon::createFromFormat('Y-m-d', $value)->startOfDay();
+            return Argon::createFromFormat('Y-m-d', $value)->startOfDay()->tz($tz);
         }
         
         $format = $this->getDateFormat();
@@ -598,7 +601,7 @@ protected function asDateTime($value)
         // Finally, we will just assume this date is in the format used by default on
         // the database connection and use that format to create the Carbon object
         // that is returned back out to the developers after we convert it here.
-        return Argon::createFromFormat($format, $value);
+        return Argon::createFromFormat($format, $value)->tz($tz);
     }   
 
     /** 

@arvislacis
Copy link
Contributor Author

@mjauvin Yes, I can confirm that your solution works without problems (in Event and Theme log) and it's better one as it solves this issue at Model level.

@mjauvin
Copy link
Member

mjauvin commented Jun 10, 2022

Feel free to submit a PR for the Model class!

@arvislacis
Copy link
Contributor Author

@mjauvin I did some additional tests on Database/Model.php changes and it doesn't seem to be ok..., date & time is ok in inner Detail page but in the all event list now the datetime is shifted by Timezone value..., also, if new event is added to the list then it seems that timezone value is calculated twice...

The initial problem (#535) was only about incorrect datetime in Event Detail page, lists were working fine.

@mjauvin
Copy link
Member

mjauvin commented Jun 10, 2022

Ok, this might be a breaking change then, please ignore my changes.
@LukeTowers @bennothommo any feedback on this?

@bennothommo
Copy link
Member

@arvislacis @mjauvin we would want the dates and times to be stored as UTC in the database. If they are retrieved at the model level as UTC and then converted to a particular timezone, that would be fine, as long as any subsequent save has the date/time converted back to UTC.

Personally, given that the Backend timezone is specifically a backend preference, I think it would be better to keep the timezone shifting out of the model, because that would then happen everywhere - including on the CMS side. Hence, I would prefer the changes happen in the partials as per @arvislacis' original changes.

@bennothommo bennothommo added maintenance PRs that fix bugs, are translation changes or make only minor changes needs review Issues/PRs that require a review from a maintainer labels Jun 11, 2022
@bennothommo bennothommo added this to the v1.1.9 milestone Jun 11, 2022
@bennothommo bennothommo added Status: Testing Needed and removed needs review Issues/PRs that require a review from a maintainer labels Jun 11, 2022
@LukeTowers
Copy link
Member

We should probably use the \Backend::dateTime($carbon) helper instead here.

@arvislacis
Copy link
Contributor Author

@LukeTowers In the last commit made changes to use \Backend::dateTime($carbon). In additional this change has a benefit that it outputs Event time in local datetime format (previously it was always outputted in English, even I don't have English language/locale set in the back-end preferences).

@arvislacis
Copy link
Contributor Author

Anything specific needed for this to be merged?

I think my last commit - ed29d81 - on this contains pretty simple fix which should no cause any problems.

@LukeTowers LukeTowers merged commit e211bed into wintercms:develop Jun 26, 2022
@LukeTowers
Copy link
Member

Looks good, thanks @arvislacis!

LukeTowers added a commit that referenced this pull request Jun 29, 2022
* wip/1.2: (21 commits)
  Added fix to ensure correct normalization and return (#588)
  Outputs time respecting backend preferences (#572)
  Bump minimum Laravel version to 9.1
  Fix site relative partial paths failing in 1.2 (#587)
  Switch back to using Laravel CacheServiceProvider
  Rebuild Snowboard agian
  Revert "Rebuild Snowboard"
  Rebuild Snowboard
  Allow a string selector for the form in a request
  Allow a string selector for the form in a request
  Use correct line breaks for Windows tests
  Backport ViewMaker tests from 1.2 branch
  Add additional testMakePartial cases to ViewMaker unit tests (#586)
  Added replaced plugins to the normalize map to ensure classloader namespace aliasing detects replacements (#585)
  Split tests into relevant module folders
  [FIX] Fix plugin flags not loading from cache correctly (#582)
  farsi spelling correction (#579)
  Moved plugin replacement namespace aliasing into register replacement method (#580)
  fix typo
  Add Winter 1.2 as version option in bug report
  ...
LukeTowers added a commit that referenced this pull request Jul 15, 2022
* develop: (65 commits)
  Update maximum supported PHP version for 1.1 (#603)
  Update EditorConfig link (#602)
  Apply consistent path normalisation to fix Windows test
  Fix view maker unit test for Windows
  Outputs time respecting backend preferences (#572)
  Rebuild Snowboard agian
  Revert "Rebuild Snowboard"
  Rebuild Snowboard
  Allow a string selector for the form in a request
  Use correct line breaks for Windows tests
  Backport ViewMaker tests from 1.2 branch
  farsi spelling correction (#579)
  Add Winter 1.2 as version option in bug report
  Adjust early termination of requests from success/error
  Increase stale check to 6 months
  Let composer resolve current php process in scripts (#563)
  Fix track input not keeping timeout
  Add jQuery AJAX prefilter as early as possible
  Add additional debugging for errors thrown in events
  Abstract Snowboard plugin constructors and destructors (#561)
  ...

# Conflicts:
#	composer.json
@arvislacis arvislacis deleted the backend-timezone-event-log branch September 9, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance PRs that fix bugs, are translation changes or make only minor changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Event Log details page title section displays date in UTC instead of user's timezone

4 participants