Skip to content

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

Merged
taylorotwell merged 6 commits into
laravel:9.xfrom
timacdonald:withoutTimestamps
Sep 15, 2022
Merged

taylorotwell merged 6 commits into
laravel:9.xfrom
timacdonald:withoutTimestamps

Conversation

@timacdonald

@timacdonald timacdonald commented Sep 15, 2022 •

Copy link
Copy Markdown
Member

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()])
);

@taylorotwell
taylorotwell merged commit 0432012 into laravel:9.x Sep 15, 2022
@marcovo

marcovo commented Sep 15, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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

@timacdonald

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@moisish

moisish commented Sep 30, 2022 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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.

7 participants