search

influxdata / influxdb-client-php

InfluxDB (v2+) Client Library for PHP
https://influxdata.github.io/influxdb-client-php/
MIT License
150 stars 47 forks source link

refactor: Split guzzle out of API classes #69

Closed SMillerDev closed 3 years ago

SMillerDev commented 3 years ago

Proposed Changes

This changes the setup of the create/write/health API classes so that the API does not strictly depend on the implementation detail that is the Guzzle client. As an example the CurlApi classes are added that I will extend in this or a future PR.

Checklist

bednar commented 3 years ago

@SMillerDev, Thanks for your PR 👍.

Which other implementations except CURL are possible?

SMillerDev commented 3 years ago

Currently none, but people could extend this work to make the client compatible with their library of choice.

SMillerDev commented 3 years ago

I renamed the commit to comply with the semantic messages but that seems to have broken the check.

bednar commented 3 years ago

It looks like problem with CircleCI webhooks. Could you try:

sign out of Github in the browser, sign out of CircleCI, then log back into CircleCI via Github.

SMillerDev commented 3 years ago

I've never logged into CircleCI so I can't really try that. But I do see checks show up now.

bednar commented 3 years ago

I've never logged into CircleCI so I can't really try that. But I do see checks show up now.

Yeah, but build isn't triggered. I have to figure out why.

SMillerDev commented 3 years ago

Service Under Maintenance

https://status.circleci.com/

SMillerDev commented 3 years ago

All but the nightly check pass now, but I don't think that one is because of my changes.

bednar commented 3 years ago

All but the nightly check pass now, but I don't think that one is because of my changes.

Could you please rebase your sources with master? The #71 contains fix for nightly build.

SMillerDev commented 3 years ago

Ah yes, everything is green now.

bednar commented 3 years ago

@SMillerDev, do you want to continue with this PR?

SMillerDev commented 3 years ago

Yeah, I'm still committed to finishing this. Just haven't found any time to work on it along my daily work.

bednar commented 3 years ago

Thanks for info, let's keep this waiting for you 😄

SMillerDev commented 3 years ago

Okay, I found some time I just have no idea how the Test parameters work. I was thinking of something like this

public function setUp($url = "http://localhost:8086", $logFile = "php://output", $guzzle=true): void

But I can't find any documentation on how it's supposed to work/be triggered.

bednar commented 3 years ago

@SMillerDev, tests depends on the running InfluxDB, so you can use something like:

# start InfluxDB (requires `docker`)
make start-server

# start tests
composer run test
SMillerDev commented 3 years ago

Just to clarify, I'm wondering how to pass/change these parameters to the setUp function. Because if that's possible I think I just make it run twice, once with Guzzle and once with cURL.

bednar commented 3 years ago

You can use system env:

public function setUp($url = "http://localhost:8086", $logFile = "php://output"): void
{
        // Initialize HTTP API via sys env
        switch ($_ENV['CLIENT_HTTP_API'] ?? 'Guzzle') {
            case "Guzzle":
                ...
            case "Curl":
                ...
        }
...
}

and then update CircleCI configuration to: config.yml.zip.

Unfortunately, I don't know of a better solution... 😞

bednar commented 3 years ago

This PR has been closed because it has not had recent activity. Please reopen if this PR is still important to you and you want to continue with them.

bednar commented 2 years ago

Hi @SMillerDev,

We are preparing a new major version of the client (v3.0.0) which add supports for custom HTTP clients. Please try the preview version by:

composer require influxdata/influxdb-client-php:generic-http-client

For more info about new major version see:

Regards

SMillerDev commented 2 years ago

Thanks for the ping and all the work @bednar . I'll give it a try later.