fix: read timezone from config#186
Conversation
| @@ -0,0 +1,12 @@ | |||
| <?php declare(strict_types=1); | |||
There was a problem hiding this comment.
is this needed? I would prefer to not mixin php to the acceptance code base
There was a problem hiding this comment.
The more I think about it, the more I think the extra effort for reading the SW5 configured timezone isn't worth it. Sorry for leading you down this EnvironmentReader path with my previous comment and our short talk.
What if we apply the simplest solution we could come up with here, just a few of my thoughts (feel free to explore a simpler approach on your own / deviate from this):
- No reading (or guessing) from the source system at all
- Premapping has a option for setting the "storage timezone" of the source system, which is like all other premappings put into the mapping table and can be used from there
- possible choices would be all valid timezones
- we could think about defaulting / pre-selecting "UTC", if that works for the majority of cases and otherwise the user can still adjust it and migrate again
- I still like your approach of using
Converter->convertValue(..., TYPE_DATETIME)to do the correct conversion automatically, but I would prefer if we could get rid of the extraContextparameter there and initialize the internal used source timezone differently from the mapping
My reasoning behind this:
- Doing a full "read environment" call is expensive and should not really be necessary. I previously thought we might store the values of that somewhere in the DB but I was wrong about that (turns out we store it on the
swag_migration_runbut at the premapping stage we don't have a run yet) - The other premapping readers also sometimes read from the source system, but they only use a dedicated
$gateway->readTableAPI. In our case we don't need DB data but rather configuration values stored as a file. Introducing extra APIs for that seems not worth the effort - Reading the SW5 config from a PHP file is error prone, e.g. they could also store the timezone in a variable and use that in multiple places
'timezone' => $timezoneor other strange things - (Not sure about this one) but is that
config.phpreally the single source of truth for this configuration in SW5? Is it not possible to override / provide config values via environment variables or other ways, where thisconfig.phpvalue could be wrong / unused? - The only reliable way I see would be asking the SW5 backend itself (e.g. they executed
config.phpand can tell what value is "active"), but that wouldn't work for our local gateway and doesn't seem worth the effort just for giving the user a little more convenience
| $connectionId = $this->migrationContext->getConnection()->getId(); | ||
| if (!\array_key_exists($connectionId, $this->timezoneCache)) { | ||
| $this->timezoneCache[$connectionId] = $this->mappingService->getValue( |
There was a problem hiding this comment.
In addition to what Lars wrote that its not a great idea to have a cache without reset interface in this parent class:
We are only ever dealing with a single connection during the whole migration process. Means there is no point in caching values "per connection", so your cache could just be a ?string instance variable instead of an associative array
There was a problem hiding this comment.
Additionally, if you consider this lookup only needs to happen once per PHP process / request, I'm not even sure if its worth it to put the Context in the method signatures just for that edge case.
E.g. you could also run this lookup once elsewhere, like a dedicated service and in it's constructor, and every convertDateTime then just uses that value and consequently you don't need to pass in the Context at all, means you also don't need error handling when the developer forgots to pass it and its not really a breaking change anymore 🤔
4 participants
closes: shopware/shopware#14186