Warn against using models.permalink

[Donald Stufft]

The permalink decorator should be deprecated and ultimately removed.

It was introduced to solve the problem of having to hardcode urls into get_absolute_url. However it violates one of the major rules of good decorators in that in forces the function signature to change in order to deal with the fact it's been decorated. Additionally it does not provide any useful functionality over using reverse() directly within the body of get_absolute_url.

[Donald Stufft]

This has been fixed in ​https://github.com/django/django/pull/377

[Anssi Kääriäinen]

+1 to deprecation.

I added some comments into the pull request, minor update needed into the docs IMO.

[Anssi Kääriäinen]

I am accepting this as deprecating functionality which can be achieved in cleaner ways is commonly done in Django (user refactor + get_profile() comes to mind here).

[Anssi Kääriäinen]

One more thing: It seems docs/ref/models/instances.txt needs to be updated. It recommends use of permalink, and it doesn't use reverse() in get_absolute_url().

[Preston Holmes]

+1 to the deprecation

A note is needed in the deprecation timeline doc.

Something needs to be done with regressiontests/model_permalink/tests.py

Generally catching the warnings is the more conservative thing during deprecation, as regressions on deprecated features are still regressions until the deprecated feature is out.

[Preston Holmes]

FTR - as pointed out by dstufft on IRC I was premature with my comments RE catching warnings. This doesn't apply to the first release of deprecation, but the second.

[Donald Stufft]

I've added a note to the deprecation timeline and pushed to my branch.

[Preston Holmes]

I'm going to mark this patch as RFC, I've reviewed - test built the docs, it is technically ready, but I'm leaving it open for a bit to give anyone else a chance to weigh-in on the decision of deprecation

[Russell Keith-Magee]

Yeah... I'm gonna weigh in on that :-)

I'm in complete agreement that models.permalink doesn't behave as a good decorator. However, it's also pretty well entrenched in Django usage.

I'd be pained to remove it purely as part of some sort of move to "API purity". It's going to force a lot of code churn on a *lot* of projects, with no real benefit.

Removing examples from the docs? Sure. Annotating the docs to say "This probably isn't the best way to do reversal"? Sure. But fully deprecating -- that seems like overkill to me.

Now, if there were to be some sort of greater benefit from this deprecation -- say, finally getting a resolution to the Replacing get_absolute_url proposal, then I might be convinced otherwise.

[Donald Stufft]

I think that if it doesn't get removed, at the very least it should trigger some form of warning for any usage of it. Maybe a longer deprecation process would be appropriate.

The code churn I think is overblown as it a easy change to make moving from models.permalink to reverse and requires very little thought or effort it likely even completely scriptable. It's not the first example of deprecation of something that the removal provides "no benefit", other similar examples would be:

1. Removal of csrf_response_exempt() and csrf_view_exempt(), one of which was no-op and the other of which was a synonym for another decorator.
2. XMLField, which was essentially a renamed TextField since the oldforms removal, and is even seeing an accelerated removal.
3. Older ways of calling page_cache will be removed.
4. django.conf.urls.defaults will be removed in favor of django.conf.urls
5. The old setup_environ() and execute_manager() functions will be removed making any manage.py created pre 1.4 broken.
6. The compat shim of HttpRequest.raw_post_body will be removed as it was a synonym for HttpRequest.body

Pretty much all of these have no harm, or little benefit for breaking backwards compatibility (via the deprecation process) other than API purity and having there be one way of doing something. Since the Permalink decorator is basically a wrapper around reverse, numbers 1, 4, and 6 are especially similar to this issue. I don't see a reason to keep around an easily replaced decorator when the alternative is documenting "yea we have this decorator, here's how to use it, but don't do that because it's bad".

Note I don't mean to be snarky sounding (I feel like I am), I just don't see how this is any different than any of these other changes, some of which are going to cause issues on a *lot* of projects too.

[Preston Holmes]

To me it is less about technical purity of the decorator implementation - but about cognitive load to those learning Django. If I'm new and encounter both reverse, and permalink, I'm going to assume that permalink, being more specific, must do something special. I'm now going to learn and remember two patterns and syntax for something instead of one.

[Anssi Kääriäinen]

I was marking this accepted by what dstufft says in comment:12 - it seems we have deprecated features even if it would not be necessary for technical reasons.

There is a lot of point in not deprecating stuff, too. We don't have long term releases, and any deprecation is at least a minor burden for those upgrading from older versions of Django. Users are forced to do those upgrades if they want security updates directly from Django.

If we want to cause less pain for upgrades, then how about user.get_profile() deprecation? I don't see that as something we must absolutely do, and it is going to cause even more pain than deprecation of permalink.

I am not +/- anything on deprecation of user.get_profile() or permalink as individual items. In general, I am in favour of deprecating less stuff. But, we should be consistent in how we deprecate stuff.

I am kind of liking deprecating stuff but not removing it after two releases. That is, we should have a longer deprecation period for API purity deprecations.

[Russell Keith-Magee]

Replying to dstufft:

I think that if it doesn't get removed, at the very least it should trigger some form of warning for any usage of it. Maybe a longer deprecation process would be appropriate.

...

Note I don't mean to be snarky sounding (I feel like I am), I just don't see how this is any different than any of these other changes, some of which are going to cause issues on a *lot* of projects too.

No argument that we've deprecated some things for 'purity' reasons in the past. However, of the examples you've provided, (4) (and maybe (6)) are the only two that are really comparable in my mind. The others are either removing features that weren't being used, or shouldn't have been used, or were internals where we documented the change because some people were known to be using them, or were so fundamentally broken that the "fix" was to remove the wrong ways to use them.

(4) is a case of something in widespread use, where we are deprecating the use of an anti-pattern (import *). We did this because Django is an influencer in the Python best-practices space, and Django is often people's first introduction to Python, so when our tutorial tells people to do things a bad way, it leads to problems later on.

(6) is a case where the name is fundamentally misleading. There's nothing about raw_post_body that is POST specific.

My hesitation to support deprecation comes from two places:

1) I *really* don't want to go through another round of tickets and django-users questions like we have done with the {% url %} tag. I'm already bracing for the urls-import onslaught. @permalink is in *really* common usage; we *will* get fallout from this.

2) There's a lingering discussion about actually fixing get_absolute_url(), which would give us a *reason* to deprecate @permalink. When this discussion is on the horizon, tinkering around the edges seems like wasted effort. If we're going to face some community pain, lets get community attention once, and fix the problem once, not attack it in lots of little passes.

If people want to remove references to @permalink everywhere except for the reference documentation, and then put a "this decorator isn't a good idea" note into the reference docs, I say go for it. But to formally deprecate it? I'd rather wait until we actually fix the problems with get_absolute_url(), and do all the changes.

[Donald Stufft]

Replying to russellm:

(4) is a case of something in widespread use, where we are deprecating the use of an anti-pattern (import *). We did this because Django is an influencer in the Python best-practices space, and Django is often people's first introduction to Python, so when our tutorial tells people to do things a bad way, it leads to problems later on.

I guess I just don't see this change as any different than that. A Decorator that forces a change in function signature is an anti-pattern as well (even more so than import * in my opinion), and like you said Django is an influencer in the Python best-practices space, and their first introduction to Python. I don't think documentation alone is enough to solve this particular anti-pattern usage because people are using it in the wild and people will still learn from the people doing it "wrong" because those people learned from Django to do it wrong.

2) There's a lingering discussion about actually fixing get_absolute_url(), which would give us a *reason* to deprecate @permalink. When this discussion is on the horizon, tinkering around the edges seems like wasted effort. If we're going to face some community pain, lets get community attention once, and fix the problem once, not attack it in lots of little passes.

Maybe i've just missed these discussions but that page you linked too earlier hasn't been updated in 3 years and as far as I can tell there's been no discussion on django-developers about changing get_absolute_url in about 3 years as well. Unless there's some discussion i've missed (completely possible) it doesn't appear that anyone has been motivated to try and fix the supposed problems with get_absolute_url in quite some time. This patch is also somewhat tangentially related to get_absolute_url because the docs also recommend it's use anytime you need a function that returns an url (see ​https://docs.djangoproject.com/en/1.4/topics/http/urls/#permalink).


However I think we're likely just going back and forth here, I'm unsure of what the next step would be, how do we get a decision made so we can either get this deprecated or get the docs fixed?

[Karen Tracey]

Replying to dstufft:

However I think we're likely just going back and forth here, I'm unsure of what the next step would be, how do we get a decision made so we can either get this deprecated or get the docs fixed?

Start by giving it some time for more people to notice this ticket and chime in with an opinion -- this ticket is only 2 days old. I agree with Russell on this one, I think formal deprecation of permalink will cause more pain than it is worth. I'd start by changing docs to discourage use and point people in the right direction, possibly noting that permalink may be formally deprecated at some point in the future. I don't see the anti-pattern argument as compelling for this one as for import *. Imports are are fundamental concept that newbies have to learn, decorators not so much. Newbies to Python generally aren't going to start writing their own decorators based on experience with a single decorator present in Django, whereas if import * is present in Django's tutorial they certainly may pick up that bad habit.

[Aymeric Augustin]

FWIW, at first sight, I'm not convinced that this deprecation is really useful. Updating the documentation sounds sufficient to me.

[Anssi Kääriäinen]

OK for me.

I think I can mark this back to accepted, with the meaning of "documentation only".

Is there any support for deprecation warnings for "API-purity"? This would mean raising a deprecation warning with no intention of removing the code.

[Russell Keith-Magee]

Replying to akaariai:

Is there any support for deprecation warnings for "API-purity"? This would mean raising a deprecation warning with no intention of removing the code.

I'm inclined to say no. Even if we made it a "pending deprecation" warning, which would be quiet by default, it would be competing for attention with warnings that we really want people to pay attention to. If we don't have a clear plan for making the decorator go away, I'd say it's not worth raising the warning.

[Tim Graham]

Docs only patch

[Tim Graham]

I've added a docs only patch based on the original pull request. Maybe it would be something we'd consider formally deprecating in "2.0", just a thought.

[Preston Holmes]

Looks good - thanks for following up on this one.

question I genuinely don't know the answer to - do links with the :func: directive generally include the ()? You have it both ways - interested in what the convention is.

https://code.djangoproject.com/attachment/ticket/18974/18974.diff#L136

[Tim Graham]

Grepping through the docs, it looks like the parens are more often omitted than included when using func. Sphinx adds them when building the docs of course.

[Tim Graham <timograham@…>]

In 0e3690d2309e64ce7b7ae4938d41544c25f29870:

Fixed #18974 - Warned against using models.permalink

Thanks dstufft for the draft patch.

[Tim Graham <timograham@…>]

In 42fa51c0027b1c40b897a0d51051a63962f95637:

[1.5.X] Fixed #18974 - Warned against using models.permalink

Thanks dstufft for the draft patch.

Backport of 0e3690d230 from master

[Tim Graham]

Four years later, maybe it's fine to proceed with the deprecation? ​PR

Russ said, " it's also pretty well entrenched in Django usage.", however, I haven't seen it used on any projects that I've worked with.

[GitHub <noreply@…>]

In 0083a4c:

Refs #18974 -- Deprecated @models.permalink() decorator.

[Tim Graham <timograham@…>]

In b59c0d7:

Refs #18974 -- Added stacklevel for permalink() deprecation.

[Tim Graham <timograham@…>]

In d476fa96:

[1.11.x] Refs #18974 -- Added stacklevel for permalink() deprecation.

Backport of b59c0d722d9b498e840feea6e2e88238d3dadf54 from master

[Tim Graham <timograham@…>]

In 4502489a:

Refs #18974 -- Removed @models.permalink() decorator per deprecation timeline.