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

Consideration/Discussion: Allow customization of ParamValueConverterRegistry? #283

Open
filecage opened this issue Jan 23, 2025 · 1 comment
Assignees

Comments

@filecage
Copy link

Abstract

The current constructor of the Psr17Client requires an instance of RequestFactory which then requires an instance of ParamValueConverterRegistry.

As far as I understand, ParamValueConverterRegistry is responsible for converting query parameters into a usable format for the HTTP API.

RequestFactory and ParamValueConverterRegistry are both implemented as final, so there is no way to apply any customization in the userland.

Use Case

For my current use case, I would like to change the way of transmitting any time-related value to my ClickHouse instance. I am using DateTimeInterface compatible objects in my code and pass them as parameters to clickhouse, but this library strips the timezone information:

$client->selectWithParams('SELECT foo FROM bar WHERE ts < {MyDatetime:Datetime}', [
    'MyDatetime' => new DateTimeImmutable('2024-12-04T14:44:05+01:00')
]);

The parameter is being sent as

2024-12-04 14:44:05
--20c9f7b0c7e9cceb8c51a1e952f2284cfb6ed4ad
Content-Disposition: form-data; name="param_MyDatetime"
Content-Length: 19

The readme clearly states that it's my obligation to correctly handle timezones and pass them to the library in a way that the clickhouse instance can process it (and/or use the timezone conversion of the library).

However, I'd prefer to have my datetime objects converted to a unix timestamp instead (or a ISO8601 string with tz/offset information and then have Clickhouse parse that with date_time_input_format=best_effort.

Possible Solutions

  • Removing final from ParamValueConverterRegistry
  • Allow overwriting the default value converters inside ParamValueConverterRegistry, either by passing them to the constructor or by adding a setConverter (string $type, callable $closure) method (personally preferred)

Obviously, there are more possible solutiouns, but these seem to be the least intrusive to me.

Let me know what you think, I'm happy to provide a PR.

@simPod
Copy link
Owner

simPod commented Jan 23, 2025

Hi!

the ParamValueConverterRegistry was designed with intention to allow extension.

I'm definitely open to allow passing converter overrides via constructor and then just array_merge it with default one.

I'll look at the timezone thingie later (tomorrow?)

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

No branches or pull requests

2 participants