Add returning methods to Eloquent Builder

[bastien-phi]

This PR add the ability to call *Returning methods on Eloquent Builder and retrieve the results as models.

The tests are pretty minimal but should be a good starting point.

The implementation is based on Builder::get (https://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Eloquent/Builder.php#L669-L700)

I don't know if adding eager loading ability should be done but it could be added here or on another PR if needed.

[tpetry]

Thanks for your work. I will review and extend it later to add more tests, but it's already looking quite good.

When do you believe eager loading should be done? I can't currently imagine a use-case where these methods are used together with eager loading. Maybe I am just not creative enough.

[bastien-phi]

For deleteReturning, insertOrIgnoreReturning, insertReturning and insertUsingReturning, I don't see any case where eager loading should be done. For the other ones, we can imagine some cases where the developer would want to eager load some relations...

In any case, as the result is an Eloquent Collection, it's still possible to eager load the relations with load :

- $user = User::query()->with('profile')->updateReturning(...);
+ $user = User::query()->updateReturning(...)->load('profile');

Maybe a first implementation like this is enough and we should wait to see if the developers really miss this feature

[tpetry]

Maybe a first implementation like this is enough and we should wait to see if the developers really miss this feature

That's a good approach 👍

[tpetry]

The deleteReturning method is very special. Should the exists property set to false before returning the records? Because the record do not exist anymore, which should be reflected on the model?

[tpetry]

Models using the SoftDeletes trait need special handling for the deleteReturning method. I will look-up on how to detect this within an Eloquent\Builder and change the method.

[bastien-phi]

The deleteReturning method is very special. Should the exists property set to false before returning the records? Because the record do not exist anymore, which should be reflected on the model?

Definitely, it would do the same as the framework does : https://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Eloquent/Model.php#L1334-L1344

[bastien-phi]

Models using the SoftDeletes trait need special handling for the deleteReturning method. I will look-up on how to detect this within an Eloquent\Builder and change the method.

Not sure how to handle that special case because the builder could reference model that need to be soft deleted as well as models that need to be hard deleted. Tricky question here, I will check what Laravel does in this case. Maybe we should add a forceDeleteReturningand just update the deleted_at timestamp in deleteReturning when the model uses SoftDeletes

[tpetry]

I‘ve got an implementation 80% ready, will finish it later. I am just checking whether the trait is used and the current value of forceDeleting

[bastien-phi]

Maybe we should add a forceDeleteReturningand just update the deleted_at timestamp in deleteReturning when the model uses SoftDeletes

I think this is the way to go. I've been tinkering with soft deletes and this is exactly what Laravel does :

it('soft deletes user via model', function (): void {
    $user = User::factory()->createOne();

    Date::setTestNow('2022-06-01 00:00:00');
    $user->delete();

    expect($user)
        ->exists->toBeTrue()
        ->deleted_at->toEqual(now());
});

it('soft deletes user via builder', function (): void {
    $first = User::factory()->createOne();
    $second = User::factory()->createOne(['deleted_at' => '1970-01-01 00:00:00']);

    Date::setTestNow('2022-06-01 00:00:00');
    User::query()->withTrashed()->delete();

    expect($first->fresh())
        ->exists->toBeTrue()
        ->deleted_at->toEqual(now());

    expect($second->fresh())
        ->exists->toBeTrue()
        ->deleted_at->toEqual(now());
});

it('hard deletes user via model', function (): void {
    $user = User::factory()->createOne();

    $user->forceDelete();

    expect($user)
        ->exists->toBeFalse();
});

it('hard deletes user via builder', function (): void {
    $first = User::factory()->createOne();
    $second = User::factory()->createOne(['deleted_at' => '1970-01-01 00:00:00']);

    User::query()->withTrashed()->forceDelete();

    expect($first->fresh())->toBeNull();
    expect($second->fresh())->toBeNull();
});

I am just checking whether the trait is used and the current value of forceDeleting

Not sure we need to check the value of forceDeleting as it belongs to the model and not the query builder.

Some pseudo-code could be something like

function deleteReturning() {
    if ($this->model->usesSoftDeletes()) {
        return $this->updateReturning(['deleted_at' => now()]);
    }
    return $this->forceDeleteReturning();
}

function forceDeleteReturning() {
   return $this->hydrate($this->getQuery()->deleteReturning())
        ->each(function($model) {
            $model->exists = false;
        });
}

Let me know if I can help !

[tpetry]

I greatly appreciate your help to research Laravel's behavior. This reduces my time effort drastically! ❤️

[tpetry]

We'Ve completely missed the eloquent timestamp @bastien-phi. Some of the original methods we are re-implementing (the one without Returning suffix) do update the timestamps.

Can you help by researching the timestamp attributes set by the respective EloquentBuilder queries?

[bastien-phi]

I'll take a look on that tomorrow. If I get some time I will push some code. Feel free to push your current implementation

[tpetry]

Just the information is enough, then I can merge it with my code. The current state is not pushable 😅

[bastien-phi]

As expected, EloquentBuilder::update updates the updated_at timestamp. It is handled by EloquentBuilder::addUpdatedAtColumn https://github.com/laravel/framework/blob/edb438abf88cf4d272c31841f7d2f9ac29d2d86e/src/Illuminate/Database/Eloquent/Builder.php#L997-L1000

When soft deleting, the updated_at is also updated so having

function deleteReturning() {
    if ($this->model->usesSoftDeletes()) {
        return $this->updateReturning(['deleted_at' => now()]);
    }
    return $this->forceDeleteReturning();
}

still make sense.

When deleting or forceDeleting, the updated_at is not updated so forceDeleteReturning should not update the property either.

Finally, for upsert laravel also does what we expect : setting updated_at to current time and created_at to current time for newly created models :

it('upserts updating timestamps', function (): void {
    Date::setTestNow(now()->milliseconds(0));
    User::factory()->createOne([
        'email' => 'taylor@email.com',
        'created_at' => now()->subDay(),
        'updated_at' => now()->subDay(),
    ]);

    User::query()->upsert(
        [
            [
                'first_name' => 'Taylor',
                'last_name' => 'Otwell',
                'email' => 'taylor@email.com',
                'password' => 'password',
            ],
            [
                'first_name' => 'Tobias',
                'last_name' => 'Petry',
                'email' => 'tobias@email.com',
                'password' => 'password',
            ],
        ],
        ['email']
    );

    expect(User::query()->where('email', 'taylor@email.com')->first())
        ->created_at->toEqual(now()->subDay())
        ->updated_at->toEqual(now());

    expect(User::query()->where('email', 'tobias@email.com')->first())
        ->created_at->toEqual(now())
        ->updated_at->toEqual(now());
});

This is handled thanks to EloquentBuilder::addTimestampsToUpsertValues and EloquentBuilder::addUpdatedAtToUpsertColumns : https://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Eloquent/Builder.php#L1010-L1029

[tpetry]

DeleteReturning

	exists = false	DELETE	updated_at	deleted_at
!SoftDeletes \|\| isForceDeleting()	x	x	-	-
timestamps = true && filled(updated_at)	-	-	x	x
default	-	-	-	x

UpdateReturning

	updated_at
timestamps=true && filled(created_at)	x
default	-

UpsertReturning

	created_at	updated_at
timestamps=true && filled(created_at) && filled(updated_at)	x	x
timestamps=true && filled(created_at)	x	-
timestamps=true && filled(updated_at)	-	x
default	-	-

These are the rules I found when testing the Laravel logic again. It's sad that all the other methods like insertReturning etc. will not update the timestams because their native Laravel counterparts are not updating the timestamps either. I could change this for the new returning statements but it feels wrong doing something different than the native functions, maybe there is a specific reason that timestamps are not updated. When those timestamps are updated in the Laravel methods I will do the same for the returning statements, for the moment I stick as close to Laravel as possible.

[tpetry]

I've finished the extended implementation.

@bastien-phi Did i miss anything?

The code can currently not be checked correctly by PHPStan, I've opened issue phpstan/phpstan#7532. The merge will have to wait until PHPStan is working correctly.

[tpetry]

Thanks @bastien-phi for the work on this Pull Request 🙏

[bastien-phi]

Glad to help !