DeleteView in Django4.0 does not call .delete() method

[Eugene Prikazchikov]

I am giving a try to Django 4.0 on one of my projects and I see that DeleteView.post does not call .delete() method anymore.

I understand that it was introduced when implementing the ticket https://code.djangoproject.com/ticket/21936
particularly this line ​https://github.com/django/django/commit/3a45fea0832c5910acee6e0d29f230f347a50462#diff-bf5815bb9e60d6b9f1a261957863a70cc9ad03efdbd7941c0e1659b7ceb2895fR250

Some codebases in the wild might assume that .delete() method get always called when DeleteView performs a deletion. I.e. .delete() method can be seen not just as a handler for DELETE http method, but as part of public interface of DeleteView - a place to specify what happens on object deletion. For example, in my project I often override .delete() method to do extra work - to make external API call or add a flash message, etc. With Django4.0 it isn't working anymore.

If it is a bug - here is my attempt with test and fix for it - ​https://github.com/django/django/pull/15055
If it is not a bug but intended behavior - perhaps it should be more clearly documented, since it might break backwards compatibility for some codebases.

[Carlton Gibson]

OK, so the change here was intended, and doc'd but likely needs calling out better in the release notes.

This was a deliberate change in #21936, finally resolved in ​PR 14634 after 8 years and (I think) 4 previous PRs.

The mainline there was:

​PR 2585 to ​PR 13362 to ​PR 14634.

The PR adjusted the inheritance structure to use FormMixin for post, allowing the confirmation step and success message. This is mentioned in the generic view docs and the ​Django 4.0 release notes.

The interplay with DeletionMixin complicated the issue significantly. This was first picked up by Caroline Simpson working on the original PR.

Happy to take input but after staring at it multiple times, for a long time, the only stable conclusion was to separate the post() HTTP handler from the delete() HTTP handler — proxying one to the other doesn't leave space for the form and messaging behaviour. (See the historical PRs for attempts in this direction.) Someone sending an API-like HTTP DELETE method request gets the old behaviour here. It's only browser based POST requests that are affected.

Continuing to proxy post() to delete(), as the PR here suggests, ends up duplicating the get_object() and get_success_url() calls (essentially from the two mixins, but inlined in this case because of the tangled inheritance structure, ​see comment.) Doing that is less than ideal: with the added FormMixin behaviour, post() is a much more complex handler than delete() — they're no longer equivalent.

On the final PR ​Mariusz and I discussed adding an extra `_delete` hook, to be called by both `delete()` and `form_valid()` but agreed is was too much API.

The correct approach is to implement your custom logic in form_valid, and per any FormMixin subclass. Or if the view is genuinely handling DELETE HTTP requests as well as POSTs, move the shared logic into a method called by both form_valid and delete (which would be the hook we decided not to add to the built-in CBV API).

I'd suggest a small addition to the release notes here, along the lines of "To accommodate this change custom logic in delete() methods should be moved to form_valid() or a shared helper method as needed."

I hope that makes sense. As I say, open to further input, but this seemed the minimum change that satisfied the desiderata.

[Carlton Gibson]

I'd suggest a small addition to the release notes here...

Pull request to that effect in ​https://github.com/django/django/pull/15059

[Eugene Prikazchikov]

I understand that with new approach you need to move that kind of logic to form_valid(), not to 'delete()` and I like that idea.

I was lucky enough to detect the broken behavior early on - the project is covered by unit tests quite well and tests started to fail.
When migrating project without much tests, you can easily miss it, your delete method will just be silently ignored. In the worst case you will know about the problem only when your users start to complain.
I am wondering if we could make it more obvious to developers that behavior has changed and their existing code must be updated.
Normally such breaking changes should be taken through deprecation process, no?

[Carlton Gibson]

Normally such breaking changes should be taken through deprecation process, no?

Yes. I'm wondering what one would look like? Hmmm 🤔

We could add an additional note to breaking changes.

(I think the new features justify the breakage, so I'd not want to revert to maintain BC here.)

[Eugene Prikazchikov]

By deprecation process I meant that initially behavior is kept the same, but the warning is emitted in runtime. In our case, when .delete method is reached during POST request, we could emit a warning like "the logic from .delete() method must be moved to .form_delete()". Thus, people should have enough time to update their code.
And then in one of following releases we change the behavior. Like described here - ​https://docs.djangoproject.com/en/3.2/howto/upgrade-version/#resolving-deprecation-warnings

Though perhaps a note in backwards incompatible changes would be enough (like ​https://docs.djangoproject.com/en/3.2/releases/1.11/#backwards-incompatible-1-11)

[Carlton Gibson]

Yes, thanks — what I meant was that having a deprecation here stops us adopting the new behaviour. (What I can't see if a way of having it both ways, defaulting to the new behaviour but falling back to the old... — I guess it'd be possible.)

[Carlton Gibson]

Hi Eugene — I've added ​an additional backwards incompatible change note on the PR.

[Eugene Prikazchikov]

OK, that sounds good.

As for:

a deprecation here stops us adopting the new behaviour.

It is not quite true. In the patch that I linked in the ticket new behavior is preserved - DeleteView still inherits from FormMixin and form_valid is still executed. All checks passed - so behavior has not changed. To prevent existing Delete views overriding delete method from breaking, in this patch form_valid() calls delete(). If we want to add deprecation warning we could extend the patch. In DeletionMixin.delete we could emit a warning if HTTP method is "POST". Here: ​https://github.com/django/django/pull/15055/files#diff-bf5815bb9e60d6b9f1a261957863a70cc9ad03efdbd7941c0e1659b7ceb2895fR211

if request.method == "POST":
    # we arrived here from .form_valid(), let's emit warning

Anyway, if you believe that updating release notes is enough - I am fine with that.

[Carlton Gibson]

Let's see what others say.

I think calling delete() isn't error free, since it re-calls the view-dispatch methods, get_object() and get_success_url().

[Eugene Prikazchikov]

Yes, I see that get_object() and get_success_url() get called twice. Not ideal, yet that should be harmless - "get_" methods usually do not have side effects,

[Carlton Gibson]

Fetching from the database is a bit of a side-effect I’d argue. 😃

But also as proposed the default implementation would raise a warning, so I’d have to override form_valid to silence that, only to delete that at the end of the deprecation period.

We can back and forth all day, let’s see what others say.

In any case, thanks for the report!

[Mariusz Felisiak]

I agree with Carlton, if we want to move forward with this feature we need to make a breaking change, it's justified. We have many previous attempts and 8 years of discussions, and it seems to me that the chosen path is the best one. Extra clarifications in release notes should suffice.

[Claude Paroz]

I've audited my different projects and saw that almost all of them are affected. I'm afraid this might break badly some apps.

Isn't it possible to detect if a subclass is overriding delete and warn in that case?

[Mariusz Felisiak]

Replying to Claude Paroz:

Isn't it possible to detect if a subclass is overriding delete and warn in that case?

Yes it should be possible, Can you take a look at ​PR?

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2c01ebb:

Refs #33263 -- Expanded release notes for DeleteView adopting FormMixin.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3151daaa:

[4.0.x] Refs #33263 -- Expanded release notes for DeleteView adopting FormMixin.

Backport of 2c01ebb4be5d53cbf6450f356c10e436025d6d07 from main

[GitHub <noreply@…>]

In 6bc437c0:

Refs #33263 -- Added warning to BaseDeleteView when delete() method is overridden.

Follow up to 3a45fea0832c5910acee6e0d29f230f347a50462.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 45de30dc:

[4.0.x] Refs #33263 -- Added warning to BaseDeleteView when delete() method is overridden.

Follow up to 3a45fea0832c5910acee6e0d29f230f347a50462.
Backport of 6bc437c0d82675ebe6aa92c8e249892205c316ef from main

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0030814:

Refs #33263 -- Removed warning in BaseDeleteView when delete() method is overridden.