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

[11.x] Add deleteIf, forceDeleteIf and restoreIf methods #51254

Draft
wants to merge 1 commit into
base: 11.x
Choose a base branch
from

Conversation

jbrooksuk
Copy link
Member

@jbrooksuk jbrooksuk commented Apr 30, 2024

This PR introduces three new methods to the Model and SoftDeletes.

  • deleteIf
  • forceDeleteIf
  • restoreIf

Oftentimes, I'll want to conditionally delete or restore a model and so this saves writing that additional wrapping condition (when will PHP get primitive if and unless?!).

Like all good Laravel methods, these methods use the value helper underneath, so you can pass a bool or Closure.

@henzeb
Copy link
Contributor

henzeb commented Apr 30, 2024

Can't it be done with Conditional? When / unless ?

@jbrooksuk
Copy link
Member Author

jbrooksuk commented Apr 30, 2024

@henzeb yes, but then you would end up with:

$model->when(true, fn () => $model->delete());

vs.

$model->deleteIf(true);

@devfrey
Copy link
Contributor

devfrey commented May 1, 2024

If we were to add an If/When/Unless variant to every framework method I wrap in a conditional statement we'd have a framework twice the size. Where does it end? Let's just embrace the PHP language and write if statements.

@jbrooksuk
Copy link
Member Author

jbrooksuk commented May 1, 2024

@devfrey I absolutely agree, and @taylorotwell said the same thing to me when I initially proposed this. However, you're more likely to want to conditionally delete or restore a model than you are to conditionally render a view or drop a field etc, where at that point, you do need to draw a line.

We do need to draw a line somewhere, but I do believe these three methods should be in the accepted side.

@taylorotwell taylorotwell marked this pull request as draft May 1, 2024 15:39
@NathanaelGT
Copy link
Contributor

@henzeb yes, but then you would end up with:

$model->when(true, fn () => $model->delete());

vs.

$model->deleteIf(true);

You can shorten it with

$model->when(true)->delete();

@insathroof
Copy link

This PR just removes the need for an if/else, it's still useful. A much more useful PR than this was recently denied. If this is approved, I would ask you @jbrooksuk to recreate this PR: #51073, because Taylor will definitely approve it. It doesn't make sense for Taylor to act like this, choosing PR for its author, but if it's the only way, fine.

@henzeb
Copy link
Contributor

henzeb commented May 1, 2024

This PR just removes the need for an if/else, it's still useful. A much more useful PR than this was recently denied. If this is approved, I would ask you @jbrooksuk to recreate this PR: #51073, because Taylor will definitely approve it. It doesn't make sense for Taylor to act like this, choosing PR for its author, but if it's the only way, fine.

What wrong with the following:

$model->when(true)->delete();

?

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

5 participants