Add backwards compatibility tip for new behaviour of formset.save(commit=False)

[django@…]

I just stumbled across the small 'Changed in Django 1.7' message at ​https://docs.djangoproject.com/en/dev/topics/forms/formsets/#django.forms.formsets.BaseFormSet.can_delete regarding formset.save(commit=False)

I have a worry that for people upgrading to 1.7 this might be overlooked. I know it's in the changelog as well, under 'miscellaneous', but should it come under 'backwards incompatible changes' or the such like? Just to make it more obvious.

Although it appears to be a small change, it's quite a big, backwards incompatible one really.

Use Case (untested):

Django Admin "Mother" form with "Child" formset. I've overridden
def save_formset() say to make sure that each 'Child' formset, when saved, has the correct 'Father' assigned.

def save_formset(self, request, form, formset, change):
    children = formset.save(commit=False)
    mother = form.save(commit=False)
    for child in children:
         child.father = mother.husband
         child.save()

As you can see, upgrading to 1.7 any children removed from the admin formset will no longer be deleted from the database.

My code for 10.7 would look like:

def save_formset(self, request, form, formset, change):
    children = formset.save(commit=False)
    mother = form.save(commit=False)
    for deleted_child in formset.deleted_objects:
         deleted.delete()
    for child in children:
         # Don't know if this check is required, haven't read the code
         if child in formset.deleted_objects:
              continue
         child.father = mother.husband
         child.save()

OK, fine. But this code is backwards incompatible
Try running this code on Django 1.6 and you'll raise [this](​https://github.com/django/django/blob/master/django/db/models/base.py#L742) error, because save(commit=False) does delete the object from the database, giving the in-memory object a pk = None

I've set this as a 'release blocker' since it's easy to fix, and will certainly make life easier for those updating if rectified

[Tim Graham]

"Miscellaneous" is a subsection of "Backwards Incompatible Changes".

I'm not sure about raising a warning. Do we have a precedent for similar changes? Unlike a DeprecationWarning where you can update your code to silence the warning, I don't see a way to do that here besides using something like warnings.filterwarnings().

When one reads the release notes, I'd think it should be pretty easy to search your code for save(commit=False) and see if it needs to be updated.

[django@…]

I did have Django's new system check framework in mind when I created this, but I don't know how you'd remove the warning, as you say.

Maybe the note on save(commit=False) should mention something like:

# save(commit=False) doesn't delete formsets in Django 1.7+
objects = formset.save(commit=False)
# You then have to delete them yourself...
# To maintain backwards compatibility:
try:
    # For Django 1.7+
    for recurring_order in formset.deleted_objects:
        recurring_order.delete()
except AssertionError:
    # Django 1.6 already deletes the objects, so don't try deleting them again.
    pass

[Tim Graham]

As far as I know, the system check framework is not designed for a situation like this. Unlike models, forms are not registered in a central location... nor is the check framework designed to track what methods are called on objects.

The improvement to the existing note seems reasonable. Have you tested it? Is AssertionError is raised when trying to delete an object that's already been deleted?

[django@…]

All sounds reasonable.

AssertionError is most definitely raised. If you look at the link I gave in the first issue post: (which it appears I got wrong)
again : ​https://github.com/django/django/blob/master/django/db/models/base.py#L739

This is the line that raises the assertion error. It's because the 1.6 save(commit=False) does delete the model, and whilst doing so sets the pk of the object to None
(I can confirm this from ​https://github.com/django/django/blob/master/django/db/models/deletion.py#L294 )

[Russell Keith-Magee]

Confirming @timo's intuition - this isn't what the system check framework is for. System check doesn't do a scan of code looking for potentially problematic code usage; it just checks models and other formal declarations for problems that would prevent startup or cause known problems.

[Tim Graham <timograham@…>]

In 2f7a7842baa50d74b9d4e93180375b7ff13b43e3:

Fixed #22747 -- Add backwards compatibility tip for new behavior of formset.save(commit=False).

Thanks django at patjack.co.uk.

[Tim Graham <timograham@…>]

In 868b5e5e957eae6d88be12b2e52ca971de81ed8d:

[1.7.x] Fixed #22747 -- Add backwards compatibility tip for new behavior of formset.save(commit=False).

Thanks django at patjack.co.uk.

Backport of 2f7a7842ba from master

[django@…]

Thanks to all you guys. Keep up the good work.