DB batch methods: code review (v4.0.4) |
BaseBuilder::setInsertBatch($key, string $value = '', bool $escape = null) : ?BaseBuilder
For the method to work correctly, the first argument must be (sample data) PHP Code: // array of key/value pairs arrays At first, array check PHP Code: if (! is_array($key)) Someone copy the code from the BaseBuilder::set() method. :-/ Since the $value variable has the typehint 'string', all data will be converted to a string if possible or throw an exception. Ok. The $key = 'field' (is not array) and $value = 'string'; As a result, we get: $key = ['field' => 'string']; Does this array match the sample data? No! This means the second parameter of the method ($value) is meaningless If the array check fails, the method should be stopped. Return $this or throw exception. Missing subarray check 1 PHP Code: $keys = array_keys($this->objectToArray(current($key))); Missing subarray check 2 Ok. The fist element the $key is array. Second? PHP Code: $key = [ We can compare two arrays like $a === $b PHP Code: sort($keys); $row = $clean; For what? PHP Code: $clean = []; BaseBuilder::setUpdateBatch($key, string $index = '', bool $escape = null) : ?BaseBuilder Missing subarray check. Hello. I am the second parameter of the method. My name is $index. Why am I here? Who am I? I guess I was copied from the BaseBuilder::set() method? Meaningless method parameter Both methods Method return type * @return $this|null Method chaining? Forget. We can return null to you! Variable names. Bad: $key, $v, $v2, $k2, $row Good: $data, $fields, $pairs, $field, $value BaseBuilder::updateBatch(array $set = null, string $index = null, int $batchSize = 100) Can $index be null? No, this is a required parameter Can $set be null? Yes. We can use the BaseBuilder::setUpdateBatch() method Can $set be an object? Yes. The data is prepared in the BaseBuilder::setUpdateBatch() method. Maybe like this? * @param array|object|null $set BaseBuilder::updateBatch(string $index, $set = null, int $batchSize = 100)
I didn’t follow all this but you seem to have your head around it, so if you want to submit a Pull Request it might be easier to review.
(12-29-2020, 05:49 AM)MGatner Wrote: I didn’t follow all this but you seem to have your head around it, so if you want to submit a Pull Request it might be easier to review. Breaking Changes https://github.com/codeigniter4/CodeIgni...ng-changes For this reason, a pull request to move the database drivers to the "Drivers" directory was denied. This is a simple operation without breaking backward compatibility. But... Many pitfalls. Some changes may break backward compatibility. I myself do not yet fully understand how serious the consequences of the changes can be. I expect the CI community to share their ideas on this topic. |
Welcome Guest, Not a member yet? Register Sign In |