Skip to content
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

refactor(param-value-converter-registry): wider date converters input type and throw exception #287

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

simPod
Copy link
Owner

@simPod simPod commented Jan 29, 2025

No description provided.

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.65%. Comparing base (a4d85be) to head (9ebb364).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/Param/ParamValueConverterRegistry.php 86.36% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
- Coverage   94.97%   94.65%   -0.33%     
==========================================
  Files          40       40              
  Lines         737      749      +12     
==========================================
+ Hits          700      709       +9     
- Misses         37       40       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@filecage
Copy link
Contributor

While I like the idea of the library handling the timezone issue, I have two converns. They are my personal preference and it's your library, so feel free to ignore them :)

First, my approach is to eliminate the need of timezone conversions in my application entirely, because it requires me to keep my stack settings synced, e.g. all ClickHouse instances need to be on the same timezone and my application code needs to know about this timezone.

While this isn't necessarily a hard thing to achieve, I am still aiming to avoid it by transmitting timezone relevant data as explicit as possible because even though it's unlikely to happen that a misconfigured timezone on a ClickHouse instance in a cluster messes up time data, I'd prefer if it were impossible.

Secondly, I think this is already quite a specific implementation detail. I suggested the custom value converter solution before because then it's a userland decision of how to handle DateTimeInterface values. In the end it's a design decision by Clickhouse to leave users with that problem and I explicitly chose your client library because it's lightweight.

Another approach could be a mix between this PR and #286: We allow passing custom value converters and also provide a set of timezone-conversion value converters that are not applied by default, but can be used by overwriting existing converters:

readonly final class DateTimeConverters {
    static function createWithTimezone (\DateTimeZone|null $clickHouseTimezone) {
        return [
            'date' => self::dateConverter($clickHouseTimezone),
            'date32' => self::dateConverter($clickHouseTimezone),
            ...,
    }
}

new ParamValueConverterRegistry(DateTimeConverters::createWithTimezone($myClickhouseTimezone));

What would you think about that? Users can opt in to use the library handling TZ conversions, I can opt in to have my values converted to unix timestamps.

@simPod
Copy link
Owner Author

simPod commented Jan 29, 2025

FYI I'm definitely planning to merge #286, forgot to mention that.

@simPod simPod force-pushed the timezone branch 3 times, most recently from 727f767 to c443273 Compare January 30, 2025 10:59
@simPod simPod changed the title feat: support clickhouse timezone input in ParamValueConverterRegistry refactor(param-value-converter-registry): wider date converters input type and throw exception Jan 30, 2025
@simPod simPod marked this pull request as ready for review January 30, 2025 11:07
@simPod simPod merged commit efc0cad into master Jan 30, 2025
12 of 14 checks passed
@simPod simPod deleted the timezone branch January 30, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants