Skip to content

[9.x] Add Model::withoutTimestamps(...) #44138

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

Merged
merged 6 commits into from
Sep 15, 2022

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Sep 15, 2022

Re-submission of #44005 as static methods.

API now matching the static nature of withoutTouching, but hopefully with a simpler implementation than previous attempts at this functionality.

$user = User::first();


// `updated_at` is not changed...

User::withoutTimestamps(
    fn () => $user->update(['reserved_at' => now()])
);

timacdonald and others added 6 commits September 5, 2022 17:23

Verified

This commit was signed with the committer’s verified signature.
timacdonald Tim MacDonald

Verified

This commit was signed with the committer’s verified signature.
timacdonald Tim MacDonald

Verified

This commit was signed with the committer’s verified signature.
timacdonald Tim MacDonald

Verified

This commit was signed with the committer’s verified signature.
timacdonald Tim MacDonald

Verified

This commit was signed with the committer’s verified signature.
timacdonald Tim MacDonald
@taylorotwell taylorotwell merged commit 0432012 into laravel:9.x Sep 15, 2022
@marcovo
Copy link
Contributor

marcovo commented Sep 15, 2022

I suspect this implementation will not work as expected with nested calls to withoutTimestamps(). E.g.:

User::withoutTimestamps(function () use ($user) {

    User::withoutTimestamps(
        fn () => $user->update(['reserved_at' => now()])
    );
    
    $user->update([...]); // <-- Will update timestamps
});

You can imagine one would call withoutTimestamps() e.g. in a controller and then deeper somewhere in a model, in different contexts, which would lead to this situation. I think it should be fixed to be able to cope with this situation.

@driesvints
Copy link
Member

I don't think it was ever intended to work with nested calls? @timacdonald

@timacdonald
Copy link
Member Author

I did not intend for it to be used this way. Would recommend not nesting calls to this or the other methods that work this way.

@timacdonald timacdonald deleted the withoutTimestamps branch September 16, 2022 00:12
@jasonmccreary
Copy link
Contributor

Yeah, I've run into this before. Disabling timestamps is a tricky beast and heavily dependent on the use case.

@moisish
Copy link
Contributor

moisish commented Sep 30, 2022

I had my own implementation of withoutTimestamps on trait that was working differently and I think the current approach is not very helpful.
The withoutTimestamps was a non static method that I was able to use on the existing model I am using or on an Eloquent Builder instance

On a single model

$model
    ->withoutTimestamps()
    ->update([
           'included' => true
      ]);

Updating all models by using a relationship

$tasks
            ->subTasks()
            ->tap(function (Builder $query) {
                $query->getModel()->withoutTimestamps();
            })
            ->update([
                'included' => false,
            ]);

@dennisprudlo
Copy link
Contributor

Wouldn't both example be possible like this?

Model::withoutTimestamps(fn () => $model->update([ 'included' => true ]));
SubTask::withoutTimestamps(fn () => $task->subTasks()->update([ 'included' => false ]));

@timacdonald
Copy link
Member Author

See @dennisprudlo suggestion above, and also see the previous PR where the method was not static for future discussion: #44005

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

7 participants