Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.3] Collection::only() with null returns all #15695

Merged
merged 2 commits into from
Oct 5, 2016

Conversation

ryanwinchester
Copy link
Contributor

@ryanwinchester ryanwinchester commented Sep 30, 2016

This comes in handy when using scopes or filters that can be reused for a lot of scenarios where you may or may not want to limit the results.

collect(['one' => 1, 'two' => 2, 'three' => 3])->only(null)->all(); 
// ['one' => 1, 'two' => 2, 'three' => 3]

For example, this already currently works for Collection::take(), which has come in handy:

collect(['one' => 1, 'two' => 2, 'three' => 3])->take(null)->all();
// ['one' => 1, 'two' => 2, 'three' => 3]

@ryanwinchester ryanwinchester changed the title [5.3] Collection::only() with null or '*' returns all [5.3] Collection::only() with null returns all Sep 30, 2016
@GrahamCampbell
Copy link
Member

This is a breaking change though, right?

@JosephSilber
Copy link
Member

This is more of a bugfix than a breaking change IMHO.

@ryanwinchester
Copy link
Contributor Author

ryanwinchester commented Oct 2, 2016

This is not a breaking change unless you were relying on Collection::only() throwing an exception with null...

@taylorotwell
Copy link
Member

Why would only(null) not return zero items?

@ryanwinchester
Copy link
Contributor Author

ryanwinchester commented Oct 3, 2016

@taylorotwell for the same reason Collection::take(null) returns all items 😁

Collection::only([]) already returns zero items, but Collection::only(null) throws an exception. I see null as more like "move along, don't apply, do nothing, etc"

For an example (before refactoring a bit):

This is exactly duplicated code with the only difference being only:

/**
 * @param array|null $keys
 * @param string $type  The record type to use, 'product' or 'client'
 * @return \Illuminate\Support\Collection
 */
public function getPieChartVolumeFor($type = 'client', $keys = null)
{
    $typeTotals = ($type == 'client') ? 'topClientTotals' : 'topProductTotals';

    if (is_null($keys)) {
        $total_booked = $this->{$typeTotals}->sum('booked');

        return $this->{$typeTotals}->map(function ($record) use ($total_booked) {
            return round(($record['booked'] / $total_booked) * 100, 2);
        })->flatten(1);
    }

    $total_booked = $this->{$typeTotals}->only($keys)->sum('booked');

    return $this->{$typeTotals}->only($keys)->map(function ($record) use ($total_booked) {
        return round(($record['booked'] / $total_booked) * 100, 2);
    })->flatten(1);
}

Used like this:

var pieData = {
    labels: ["ABC", "ABC-MB", "ABC-SG"],
    datasets: [{
        data: {!! $report->getPieChartVolumeFor('product', ["ABC", "ABC-MB", "ABC-SG"])->toJson() !!},
        backgroundColor: [
            "#2b90d9",
            "#9baec8",
            "#6fae50",
        ],
        hoverBackgroundColor: "#FF6384"
    }]
};

In the usage, there are other charts where I limit the data to specific items of interest, and some where I use everything.

If I could use null in Collection::only() that would make any filtering methods super simple, and to me feels intuitive.

@taylorotwell taylorotwell merged commit ccd9147 into laravel:5.3 Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants