codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.37k stars 1.9k forks source link

Bug: array_flatten_with_dots() loses emtpy arrays #5603

Closed iRedds closed 2 years ago

iRedds commented 2 years ago

What happened?

I think the behavior of the array_flatten_with_dots function is not correct. If the value is an empty array, then the key-value pair is ignored and does not get into the resulting array, which in my opinion violates the integrity of the data. The documentation also doesn't say that empty arrays are ignored.

Steps to Reproduce

An example from tests.

$array = [
    'foo' => 'bar',
    'baz' => [],
    'bar' => [
        'fizz' => 'buzz',
        'nope' => 'yeah',
        'why'  => [],
    ],
];

array_flatten_with_dots($array);
// result 
//    [
//        'foo'      => 'bar',
//        'bar.fizz' => 'buzz',
//        'bar.nope' => 'yeah',
//    ]

Expected Output

[
    'foo'      => 'bar',
    'baz'      => [],
    'bar.fizz' => 'buzz',
    'bar.nope' => 'yeah',
    'bar.why'  => [],
]
kenjis commented 2 years ago

If the value is an empty array, then the key-value pair is ignored and does not get into the resulting array, which in my opinion violates the integrity of the data.

It seems it depends on use cases. Can you show a use case?

sfadschm commented 2 years ago

What would happen if the array that is ignored here was a numerically indexed array with content? Would it be treated as a value in total or would the numbers be used as dot-keys, too?

Because I think the current issue is that the subarray that is ignored here could as well be another level of dot-keys instead of being a preservable value, therefore it is skipped, as no keys are found in it.

Long story short: All levels of arrays are flattened, so an empty array is just interpreted as an empty key list and not as a value.

kenjis commented 2 years ago

I think we need to choose one or the other.

  1. This is by design, and document it. (Do not use if you need integrity.)
  2. This is a bug, fix to get the integrity of the data.
sfadschm commented 2 years ago

I would go for documentation. I can imagine adding a designated method array_flatten_with_dots_fancy or such which could take parameters as, e.g.

int maxLevel = 0, bool emptyArrayAsValue = false, bool keysFromAssociativeArraysOnly = false.

The current Implementation would then be the default case of the method that allows for more specific flattening control.

iRedds commented 2 years ago

@kenjis I discovered this behavior while trying to fix validation error messages for wildcard fields. Using the array_flatten_with_dots function I was trying to get an array for further processing. But because the empty array is excluded from the set, the validation class tests fail.

https://github.com/codeigniter4/CodeIgniter4/blob/812af1e6285c352ae9b4da044fd7c1a0e8815d90/tests/system/Validation/ValidationTest.php#L1038-L1045

Instead of

['bar.0.foo' => 'baz', 'bar.1.foo' => []]

I get

['bar.0.foo' => 'baz']

The validation succeeds, but the test fails. Therefore, I want to know. This is a design error of the helper or its normal behavior.

iRedds commented 2 years ago

@sfadschm There is a problem with data integrity. A dot array is an alternate representation of an existing array. But if the empty array is excluded as a result of the conversion, then the original array from the dotted array can no longer be restored.

sfadschm commented 2 years ago

@sfadschm There is a problem with data integrity. A dot array is an alternate representation of an existing array. But if the empty array is excluded as a result of the conversion, then the original array from the dotted array can no longer be restored.

Ok I think I see the problem now. If the method is intended to be reversible (to convert an array forth and back), this looks like a bug.

I found this Implementation which seems right to me to keep consistency: https://github.com/adbario/php-dot-notation/blob/0a69aaabe706beebac848ef32587116b68875eea/src/Dot.php#L143-L173

sfadschm commented 2 years ago

Maybe @paulbalandan can clarify, he wrote the method.

paulbalandan commented 2 years ago

I think this is a bug. My intention then was to convert a multidimensional array into a single dimensional one. Therefore, elements should be preserved. The reversibility idea hits me that current behavior is incorrect should we have an array_explode_from_dots function.

sfadschm commented 2 years ago

array_explode_from_dots

That was my thought as well. Might actually make sense to implement that along with the bugfix, as it could be good for tests.

kenjis commented 2 years ago

@iRedds Thank you for your clear example. The consensus seems to be this is a bug.