Delete HasMany subentry when relation column is not nullable

[pxpm]

WHY

BEFORE - What was wrong? What was happening before this PR?

reported in #4187 when column is nullable the default behaviour should be the deletion when no default is provided in database. Our conditional was wrong and that was not happening.

AFTER - What is happening after this PR?

It works as expected. I also added tests! 👍

HOW

How did you achieve that, in technical terms?

refactored the conditional

Is it a breaking change or non-breaking change?

nop

How can we test the before & after?

run test suit without this PR changes (only the tests), run with all the changes.

[tabacitu]

Love the extra tests! This gives me so much confidence. Just have one small suggestion, tests run fine after the suggested change.

[tabacitu]

Let's talk a little bit about this.

Previously: we were NOT deleting entries, by default. AT ALL. Unless you specified force_delete, in which case we were 100% sure that's what the dev wants. We took the safe approach.

After this PR: We have a case where we default to deleting. So let's be careful about this. Are we 100% sure people will want to delete the entries unless $relationColumnIsNullable || $dbColumnDefault !== null? Is it intuitive that in all other cases it will delete the entries? The condition seems kind of broad so... is it possible we're defaulting to delete in some scenarios where it doesn't make sense? This is a HARMFUL action.

Can you please give me an example @pxpm ? The easiest for me to understand would be with the entities in the Demo.

[pxpm]

Hey @tabacitu

Sure. I just recall we already talked about this. And here we are again. Sorry for that, I think this is what makes sense and what I expect to happen if my column is NOT nullable and I didn't setup any column default. If I don't use force_delete => true it will throw an error because we are trying to set to null on a non nullable column. It just seems to me that it makes backpack look dumb, why would backpack try to set to null on a non nullable column where developer didn't sepecify any fallback_id or a database default $table->bigInteger('some_id')->default(0)?

STEP 1: Go to monsters and create one filling the HasMany with subfields with atleast two items:

STEP 2: Delete one of the HasMany, leave the other. Save. Check database, you have 1 item there.

STEP3: Delete all remaining HasMany items. Save. Check database.

You will see that now, instead of deleting the items, we set the key to 0. We don't have force_delete => true, nor database default.
IF WE HAD A FK IN DATABASE from monster_id to monster table, it would error in a database level instead of setting to 0, in this example it sets to 0 because monster_id is just and int column.

Let me know,
Pedro

[tabacitu]

Hmmm ok... I get it now.

But let's please change it so that the default action is non-destructive. Destructive actions should ONLY happen in that particular use case, where we know that's what we want. Otherwise do an update, which is safe.

We shoud never ever fall back to a destructive action. So maybe we do something like this:

        if (!$relationColumnIsNullable && !$dbColumnDefault) {
            return $removedEntries->delete();
        }
        
        return $removedEntries->update([$relationForeignKey => $dbColumnDefault]);

Conditional is probably not ok, but you get what I mean.

[pxpm]

@tabacitu refactored the condition.

Found a way for you to easy test when there are FK restrictions in database.

In PetShop, InvoicesCrud, remove the requirements from items in the InvoiceRequest.

Create an invoice add one item, save. Open the invoice, delete the item, try to save (it will not change to 0 as in the previous example, but it will throw an ugly db error about FK contrains).

PS: It's realistic to remove those constrains from InvoiceRequest for example if your Invoice has status: draft, it may not have any items and could be saved as a draft so you could not use required for items in the request.

[tabacitu]

Now THIS is a clean PR!

[tabacitu]

As a sidenote... another option for us was in that particular instance... instead of running a DELETE. To throw an error telling the developer to use force_delete... That would have been the "safe" choice, I think. But I don't think this is harmful either. Time will tell, I guess 😅