Optimize subscription query.

[Godofbrowser]

        $subscriptions = $this->subscriptions->sortByDesc(function ($value) {
            return $value->created_at->getTimestamp();
        });

        foreach ($subscriptions as $key => $subscription) {
            if ($subscription->name === $name) {
                return $subscription;
            }
        }

The above retrieves all subscriptions, then iterates through them. This could be expensive depending on how many rows are on table. It would be much more efficient to perform as much operations in a query, and less with PHP.

[gerardojbaez]

Hi @Godofbrowser, thanks for your PR! There is an issue with your new code IMHO. It may cause the n+1 problem when iterating over multiple PlanSubscriber and retrieving each subscription.

For example:

foreach ($user as $user) {
    $user->subscription()->name; // A new query will be generated for each user.
}

This code is commonly used when displaying a list of subscribers (in this case, users).

There should be other ways to optimize this section without introducing other issues that may impact performance.

[Godofbrowser]

Hello @gerardojbaez , you are right. The new code will produce multiple queries in that example.

However, i think the previous implementation will equally do the same if the subscriptions relation is not eager loaded.

Since eager loading is good practice, i thought of editing the code to make the subscription method a relation and then support eager loading.

// This will always pick the first sorting by created_at desc
    public function subscription($name = 'main'){
        return $this->hasOne(config('laraplans.models.plan_subscription'))
            ->where('name', $name)
            ->orderBy('id', 'desc');
    }

That way we can have just one query like the example below:

$users = User::query()->with( 'subscription')->get();
foreach ($users as $user) {
    $user->subscription ? $user->subscription->name : "Not Subscribed";
}

Also,

// Eager load the plan alongside the subscription
$users = User::query()->with( 'subscription', 'subscription.plan')->get();
foreach ($users as $user) {
    $user->subscription ? $user->subscription->plan->name : "None";
}

The above works as expected, but introducing this update means all usage of $user->subscription($name) will have to be updated to $user->subscription($name)->first() or simply $user->subscription.

Although, this isn't a priority but i'll like to know what you think about making the method a relation. Regards.