feat: Add Chain::merge functionality (Close #54)

[colorninja]

This should be a bit more efficient than the internal array_merge. It is using references in order to improve speed when using large arrays. It also does not modify passed arrays to avoid unexpected issues for consumers.

[florianeckerstorfer]

First, thanks for your contribution. You implementation does three things:

1. Create a static method to create a Chain from multiple arrays
2. Switch to a more efficient merging code
3. Support for merging more than two arrays

In #54 you describe the following scenario:

Chain::create(array_merge($object->getOutsidePhotos(), $object->getInsidePhotos()))
    ->map(fn (Photo $photo) => $photo->getUrl())
    ->map(fn (string $url) => $this....);

And propose the following solution:

Chain::merge($object->getOutsidePhotos(), $object->getInsidePhotos())
    ->map(fn (Photo $photo) => $photo->getUrl())
    ->map(fn (string $url) => $this....);

But following the philosophy of Chain I think the correct approach to implement this would be:

Chain::create($object->getOutsidePhotos())
    ->merge($object->getInsidePhotos())
    ->map(fn (Photo $photo) => $photo->getUrl())
    ->map(fn (string $url) => $this....);

This already works and ->merge() also supports recursive merging (via array_merge_recursive). With this approach you have a lot more flexibility. In your example you want to array_merge at the beginning of the chain, but there are cases where you need to map an array before you can merge it.

I don't think that having a separate static method to create to create a Chain from multiple arrays is not very useful and in fact, works against the philosophy of this library. But the other two features from your PR would be useful. I would appreciate it if you would update the PR to implement the faster merging in the Merge trait (you can leave array_merge_recursivefor the recursive merging for now). For merging multiple arrays I would suggest to create a new trait mergeMultiple or mergeMany in order to keep the API of merge backwards compatible.

[colorninja]

Thank you for the detailed explanation and the overview of my implementation. I will modify it but there is a slight concern, which is the difference of the two merging methods.

array_merge dealing with collisions:

If the input arrays have the same string keys, then the later value for that key will overwrite the previous one. If, however, the arrays contain numeric keys, the later value will not overwrite the original value, but will be appended.

Values in the input array with numeric keys will be renumbered with incrementing keys starting from zero in the result array.

Function implemented would always overwrite previous values. Also it will not renumber in any way. These changes might introduce bugs in the client's applications.

---

Example with array_merge:

$array1 = array();
$array2 = array(1 => "data");
$result = array_merge($array1, $array2);

Result:

Array
(
    [0] => data
)

---

Example with PR function:

Chain::create([])
    ->merge([1 => 'data'])

Result:

Array
(
    [1] => data
)

Should I still implement it?

[florianeckerstorfer]

@colorninja You're right, this would break compatibility. We could add an option (defaults to false) or implement it in a separate trait. What do you think?

[colorninja]

I think a separate trait would be better. The boolean would be too subtle, where a different trait will actually provoke questions upon IDE autocomplete (wondering what the difference is). I am having a hard time thinking of a intuitive name of the trait/method. Any ideas?

[colorninja]

@florianeckerstorfer What about quickMerge/fastMerge?

[florianeckerstorfer]

@colorninja Yes, either quickMerge or maybe simpleMerge. Your choice.

[colorninja]

Turns out that PHP automatically optimizes passing by reference, so no need for the &. This is nice, because now you do not have to declare a variable to pass a Chain object reference.