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

fix forceDelete product #2075

Draft
wants to merge 3 commits into
base: 1.x
Choose a base branch
from
Draft

Conversation

wychoong
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Jan 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lunar-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2025 4:06am


$record->productOptions()->detach();

$record->associations()->delete();

$record->channels()->detach();

$record->clearMediaCollection('images');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unnecessary as handled by Spatie's Media observer for cleanup

Comment on lines -23 to +25
$record->variants()->each(function (Model $variant) {
$variant->values()->detach();

$variant->prices()->delete();
});

$record->urls()->delete();

$record->variants()->delete();
$record->variants()->get()->each->forceDelete();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup to use ProductVariant observer for handling relationship


$record->productOptions()->detach();

$record->associations()->delete();

$record->channels()->detach();

$record->clearMediaCollection('images');

return $record->forceDelete();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole code could be simplified to just this one line if we register a Product observer for handling the relationships.

user land can register own observer as well, i think 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alecritson need your input on this, not sure is ProductObserver is purposely left out

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wychoong Yeah I think ultimately having this kind of logic in the panel is a mistake, we should really be using observers, whether within the model itself or in a separate class.

Copy link
Contributor Author

@wychoong wychoong Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alecritson Alright, got it.
Can you review the part for media observer? Will prepare a separate pr for the product observer

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.

2 participants