[5.4] mapWithKeys shouldn't reindex the collection

[fernandobandeira]

mapWithKeys shouldn't reindex the array when using numeric keys, added a flag to change this behavior and the tests accordingly.

Refs: #15409, #16142

This was a predicted behavior: #14430 (comment) but since people are using it for this purpose and the name is kinda misleading i thought we should address this...

Update: If this should be the intended behavior then we could add a note to the docs about it...

[taylorotwell]

I definitely don't want to be passing a reIndex flag everywhere so lets update the docs.

[vlakoff]

This might be fixable by avoiding collapse and using a simple loop, though there may be more optimal solutions.

Also take care of preserving the array order (not the same as the numerical indexes). To verify this, a foreach on the result should iterates the items in the same order.

[vlakoff]

Here is what I have currently:

collect()->macro('mapWithKeys_v2', function ($callback) {

    $result = [];

    foreach ($this->items as $key => $value) {
        $assoc = $callback($value, $key);

        $mapValue = reset($assoc);
        $mapKey = key($assoc);

        $result[$mapKey] = $mapValue;
    }

    return new static($result);
});

[fernandobandeira]

@vlakoff I'm simplifying it a little bit i'll create a PR.

[vlakoff]

I don't know if the callback returning several rows is a supported use case, but it could be useful and maybe some people do it, and more importantly to preserve BC:

    foreach ($this->items as $key => $value) {
        $assoc = $callback($value, $key);

        foreach ($assoc as $mapKey => $mapValue) {
            $result[$mapKey] = $mapValue;
        }
    }

[vlakoff]

Refs subsequent PR by fernandobandeira: #16564

[vlakoff]

I think flatMap should preserve numeric keys as well.

If there are non-consecutive numeric keys, it's because the user has specified them. So the user probably expects them to be retained…

[fernandobandeira]

Makes sense, currently

$collection = collect([
    [1 => 'Sally'],
    [3 => 'Arkansas'],
    [2 => 28]
]);

$flattened = $collection->flatMap_v2(function ($values) {
    return array_map('strtoupper', $values);
});

results into:

[
    0 => "SALLY",
    1 => "ARKANSAS",
    2 => "28"
]

Is this intended?

If it isn't then we could change flatMap and just alias mapWithKeys since it's just another syntax for the callback as it is currently, and add a test to flatMap...

[vlakoff]

we could change flatMap and just alias mapWithKeys since it's just another syntax for the callback as it is currently, and add a test to flatMap

agree.

[vlakoff]

Mmmm, wait.

$collection = collect([
    ['Sally'],
    ['Arkansas'],
    [28]
]);
$callback = function ($values) {
    return array_map('strtoupper', $values);
};
print_r( $collection->flatMap($callback)->all() );
print_r( $collection->mapWithKeys_v2($callback)->all() );

Results:

Array (
    [0] => SALLY
    [1] => ARKANSAS
    [2] => 28
)
Array (
    [0] => 28
)

Let's not touch flatMap for now.