Framework: Potential issue with collection split method

[helmut]

• Laravel Version: 5.5
• PHP Version: 7.01

Description:

Noticed a bit of an issue with the collection split method that we may want to fix. Of course this might not be an issue but my understanding of the method might be wrong.

I've got a collection with 4 items and when I run split(3) I expect to get back 3 groups. Instead I get back 2 groups.

Steps To Reproduce:

collect(['a', 'b', 'c', 'd'])->split(3);

// expected 
[['a', 'b'], ['c'], ['d']]

// actual
[['a', 'b'], ['c', 'd']]

Does anyone else agree with my view that when you split into 3 groups and you have at least 3 items in the collection it should return 3 groups?

These work as expected:

collect(['a', 'b', 'c'])->split(3);

// returns
[['a'], ['b'], ['c']]

collect(['a', 'b', 'c', 'd', 'e'])->split(3);

// returns
[['a', 'b'], ['c', 'd'], ['e']]

Here is a failing test if that helps:

    public function testSplitCollectionIntoThreeWithCountOfFour()
    {
        $collection = new Collection(['a', 'b', 'c', 'd']);

        $this->assertEquals(
            [['a', 'b'], ['c'], ['d']],
            $collection->split(3)->map(function (Collection $chunk) {
                return $chunk->values()->toArray();
            })->toArray()
        );
    }

[leonrenkema]

When I read the description from the manual (The split method breaks a collection into the given number of groups) I would also expect 3 groups.

At the end it uses array_chunk that does not support decimal sizes. There is no way to let array_chunk return three arrays when the input is an array with 4 elements.

[thecrypticace]

Yeah, the results here seem non-intuitive to me. I ran some tests (repo here: https://github.com/thecrypticace/split-example) and got these—what appear to be wrong—results:

Array of  4 + split of  3 gives  2 groups ( 66.667%)
Array of  5 + split of  4 gives  3 groups ( 75.000%)
Array of  6 + split of  4 gives  3 groups ( 75.000%)
Array of  6 + split of  5 gives  3 groups ( 60.000%)
Array of  7 + split of  5 gives  4 groups ( 80.000%)
Array of  7 + split of  6 gives  4 groups ( 66.667%)
Array of  8 + split of  5 gives  4 groups ( 80.000%)
Array of  8 + split of  6 gives  4 groups ( 66.667%)
Array of  8 + split of  7 gives  4 groups ( 57.143%)
Array of  9 + split of  4 gives  3 groups ( 75.000%)
Array of  9 + split of  6 gives  5 groups ( 83.333%)
Array of  9 + split of  7 gives  5 groups ( 71.429%)
Array of  9 + split of  8 gives  5 groups ( 62.500%)
Array of 10 + split of  6 gives  5 groups ( 83.333%)
Array of 10 + split of  7 gives  5 groups ( 71.429%)
Array of 10 + split of  8 gives  5 groups ( 62.500%)
Array of 10 + split of  9 gives  5 groups ( 55.556%)

If you run the test with a larger result set you'll see some interesting patterns emerge.

[helmut]

Yes I did suspect that there were many other potential cases where this method might fail similarly.

After a bit of searching I came upon a 'partition' function in a stack overflow (link) that I have adapted that seems to pass tests from my end. @thecrypticace would you like to see if this new method passes your tests?

/**
     * Split a collection into a certain number of groups.
     *
     * @param  int  $numberOfGroups
     * @return static
     */
    public function split($numberOfGroups)
    {
        if ($this->isEmpty()) {
            return new static;
        }

        $groups = new static;

        $groupSize = floor($this->count() / $numberOfGroups);

        $remainder = $this->count() % $numberOfGroups;

        $offset = 0;

        for ($key = 0; $key < $numberOfGroups; $key++) {
            $increment = $key < $remainder 
                            ? $groupSize + 1 
                            : $groupSize;

            $groups[$key] = new static(
                array_slice($this->items, $offset, $increment)
            );

            $offset += $increment;
        }

        return $groups;
    }

It's not pretty but might be a good starting point?

[fsasvari]

Yeah, I'm having the same problem with split() method. Trying to split following arrays:

collect(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'])->split(6);

It splits into 5 grouped arrays instead of 6.

[tvbeek]

I have created a pull request to change it and added test with the values that are provided in this issue. See: https://github.com/laravel/framework/pull/24083

[tvbeek]

This is merged in master, that means it will be fixed in 5.7