Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18974 closed Cleanup/optimization (fixed)

Warn against using models.permalink

Reported by: dstufft Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: timograham@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

18974.diff (7.4 KB) - added by timo 3 years ago.
Docs only patch

Download all attachments as: .zip

Change History (26)

comment:1 Changed 3 years ago by dstufft

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by dstufft

  • Has patch set

comment:3 Changed 3 years ago by akaariai

+1 to deprecation.

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

comment:4 Changed 3 years ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

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).

comment:5 Changed 3 years ago by akaariai

  • Patch needs improvement set

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().

comment:6 Changed 3 years ago by ptone

  • Needs documentation set

+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.

comment:7 Changed 3 years ago by ptone

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.

comment:8 Changed 3 years ago by dstufft

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

comment:9 Changed 3 years ago by dstufft

  • Needs documentation unset
  • Patch needs improvement unset

comment:10 Changed 3 years ago by ptone

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:11 Changed 3 years ago by russellm

  • Triage Stage changed from Ready for checkin to Design decision needed

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.

comment:12 follow-up: Changed 3 years ago by 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.

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.

Last edited 3 years ago by dstufft (previous) (diff)

comment:13 Changed 3 years ago by ptone

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.

comment:14 Changed 3 years ago by akaariai

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.

comment:15 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by russellm

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.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by dstufft

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?

Last edited 3 years ago by dstufft (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 3 years ago by kmtracey

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.

comment:18 Changed 3 years ago by aaugustin

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

comment:19 follow-up: Changed 3 years ago by akaariai

  • Component changed from Database layer (models, ORM) to Documentation
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

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.

comment:20 in reply to: ↑ 19 Changed 3 years ago by russellm

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.

Changed 3 years ago by timo

Docs only patch

comment:21 Changed 3 years ago by timo

  • Cc timograham@… added
  • Patch needs improvement unset
  • Summary changed from Deprecate models.permalink to Warn against using models.permalink

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.

comment:22 Changed 3 years ago by ptone

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:23 Changed 3 years ago by timo

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.

comment:24 Changed 3 years ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 0e3690d2309e64ce7b7ae4938d41544c25f29870:

Fixed #18974 - Warned against using models.permalink

Thanks dstufft for the draft patch.

comment:25 Changed 3 years ago by 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

Note: See TracTickets for help on using tickets.
Back to Top