Fix escape #87
Fix escape #87
Conversation
|
I think use preg_replace() it`s very bad, its very slow function for simple replacement |
|
As I read the code a bit, replacing the quotes is not the proper way as I think it might accidentally replace quotes you wish to have in your string. And also, talking the speed this class deserves refactoring. |
simPod
force-pushed
the
fix-escape
branch
3 times, most recently
from
e3b4272 to
b0178fe
Compare
|
@isublimity CS won't pass for missing return type hints that are unsupported in 7.0 and therefore we cannot add them. |
| @@ -2,6 +2,10 @@ | |||
| namespace ClickHouseDB\Quote; | |||
|
|
|||
| use ClickHouseDB\Exception\QueryException; | |||
| use function array_map; | |||
| use function is_string; | |||
| use function preg_replace; | |||
There was a problem hiding this comment.
Do you think this is clear code ?
There was a problem hiding this comment.
Of course. Either you can use \array_map in your code or import it. Importing it is cleaner so your actual code is not polluted with \s
There was a problem hiding this comment.
@isublimity what do you think about that ?
There was a problem hiding this comment.
I'm. It is part of the code style to reference it via use statement instead of prepending \. What is the issue you see there?
There was a problem hiding this comment.
@simPod you said "you can use \array_map in your code or import it.", I show you errors about use \ in function name
There was a problem hiding this comment.
I meant that it both have the same effect but only one is compliant with doctrine cs and that's by importing it.
There was a problem hiding this comment.
@simPod - please write me if need merge, i'm lost discussion(
sorry(
There was a problem hiding this comment.
@isublimity I think this PR is ready. I have leveraged the code used for encoding strings https://github.com/smi2/phpClickHouse/pull/87/files#diff-c29b9c40206dcbba3b525b3b3d284effL82 and reapplied it for arrays as well.
The test written by @2ur1st doesn't pass before this change so I suppose it's fixed now as it passes.
Follows #86