pkp/pkp-lib#11532 set database time zone during installation #11543
Conversation
kaitlinnewson
requested review from
jonasraoni
and removed request for
jonasraoni
|
Hi @jonasraoni, please ignore the review request for now - I see tests failing so I've got more investigating to do |
kaitlinnewson
force-pushed
the
11532-3_5_0
branch
from
1f82e5e to
21def1d
Compare
kaitlinnewson
force-pushed
the
11532-3_5_0
branch
from
21def1d to
1154472
Compare
|
Hi @jonasraoni, can you give this an initial review? Tests are now mostly passing in OJS except for one unrelated test failure, and I'm going to set up OMP/OPS tests soon. |
|
|
||
| // Get the current offset for this timezone | ||
| $dateTime = new DateTime('now', new DateTimeZone($timeZoneName)); | ||
| $offset = $dateTime->format('P'); |
There was a problem hiding this comment.
Maybe to use date_default_timezone_set (the next line below) first, before getting the offset, and then to use $offset = (new DateTime())->format('P'); (to get offset for the current time zone).
Because we say: $timeZone ?: ini_get('date.timezone') ?: 'UTC' in date_default_timezone_set -- The timeZone will probably be correctly/successfully constructed, because it comes from the install form where it is also validated, but if for some reason the timeZone is not successfully created, then other time zone will be set in PHP. So maybe it is better to calculate offset based on that, on the actually set time zone?
| $result = parent::preInstall(); | ||
|
|
||
| if ($result && $this->getParam('timeZone')) { | ||
| $this->initializeDatabaseTimeZone($this->getParam('timeZone')); |
There was a problem hiding this comment.
Do we need to consider the return value here -- if the time zone could not be set correctly in the DB, when the function returns false? Shell the installation proceed however? 🤔
|
Hi @kaitlinnewson, just two questions :-) |
No description provided.