Skip to content

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Oct 17, 2018

I tried to cleanup Transport with Client a bit. This is first part. I tried to reflect changes in docs and examples as well. Currently it seems to me that there are usualy multiple APIs for the same thing so I'm trying to clean it up and make it more clear, simple so we avoid unwanted side effects.

Eg. setTimeout and setConnectTimeout influence curl settings (CURLOPT_TIMEOUT_MS, CURLOPT_CONNECTTIMEOUT_MS). Before this PR it seemed like they controlled ClickHouse settings as they were called via eg. settings()->max_execution_time

@simPod simPod mentioned this pull request Oct 18, 2018
@isublimity
Copy link
Contributor

Big work, but:
This not good idea change all API !

  1. I cant support this version and late version, im not migrate to NEW version API
  2. Many many code need rewrite

@simPod
Copy link
Contributor Author

simPod commented Oct 18, 2018

I cleaned just a little part so the changes are not drastic. But if released under new version the API can change, am I right?

What do you mean by this?

Many many code need rewrite

@simPod
Copy link
Contributor Author

simPod commented Oct 19, 2018

If I get your point correctly, the problem is that it requires a lot of refactorings in your project?

If I can recommend, implement PHPStan that detects it for you and you no longer have to worry about it.

The thing is, we can stay conservative and do not refactor the old code. But ultimately when new code keeps being added it might result in spaghetti 🍝 code that will be very hard to maintain.

@simPod simPod force-pushed the cleanup-transport branch from 29ce0cd to e302b54 Compare October 23, 2018 09:17
@simPod
Copy link
Contributor Author

simPod commented Oct 23, 2018

This also fixes broken readonly mode for read queries that contain functions like numbers()

@simPod simPod force-pushed the cleanup-transport branch from e302b54 to 1a41c34 Compare October 23, 2018 09:20
@simPod simPod force-pushed the cleanup-transport branch from 1a41c34 to b10f9ea Compare October 23, 2018 09:34
@isublimity
Copy link
Contributor

I`m not support this pull request -> close
Why:

All previous methods and functions should remain unchanged.Must be complete backwards compatible.
No change:

public function isEnableHttpCompression()
=>
public function isHttpCompressionEnabled()
$cli->transport()
=>
$cli->getTransport()

and other ... 1000 changes names

I do not like when phpUnit tests change, for a renaming function/method.

I do not have a lot of free time to view 33 modified files per one pull request.

Info:
170 user stars this project
-30% old star
One user have 1...4 projects where phpCh ( i`m have 6 projects)
170-30% * 1.1 = 131 project must change our code ?

If you want to apply these changes, fork code and use it.

@isublimity isublimity closed this Oct 23, 2018
@simPod
Copy link
Contributor Author

simPod commented Oct 23, 2018

Yup, that's what versions are for. The argument that anyone has to change his code is invalid.

I cannot continue to contribute to this library if there's no will to improve the code base. This change is pretty small and is considered problematic already. There's no way we can make the code clean and remove most of the bugs with such approach.

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