Small decision needed on #15025
Don Spaulding
Prior to r14992 (which is the fix for #7153), the way a callable
context variable was resolved depended on where it was in the lookup
chain. Sometimes it was invoked, other times it was treated as a
first-class function. r14992 changed that behavior so that the
callable is always invoked before resolving the next bit of the
variable. I'm starting to see the other side of the coin, that it is
more consistent for the template language to behave in this manner.
The thing I can't decide is how much of a corner case my template
usage is. I certainly rely on both styles of callable resolution in
my templates. Basically, they both Just Worked for me. I never gave
"sometimes I get a callable, sometimes I get the result of calling a
callable" a second thought. I'm looking for some feedback on whether
I've just been relying on a bug in the template engine and need to fix
my templates, or whether r14992 is pulling the rug out from under my
(working, IMO) templates.
#15025: http://code.djangoproject.com/ticket/15025
r14992: http://code.djangoproject.com/changeset/14992
#7153: http://code.djangoproject.com/ticket/7153
Tai Lee
described is at:
http://djangosnippets.org/snippets/1016/
The fix for this use case is easy. Pass a list of `(button.func_name,
button.short_description)` tuples as `buttons` in the context instead
of passing a list of callables. But the problem is that a previously
working template stopped working, with no obvious reason why.
The docs say that template variables are resolved by trying a (in this
order, with shortcut logic) a dictionary lookup, an attribute lookup,
a method call and a list index lookup. Technically the docs weren't
even correct prior to [14992] because there was no shortcut between an
attribute lookup and a method call, but that's a very minor pedantic
issue. Now, it tries a dictionary lookup, an attribute lookup and a
list index lookup, and then tries to call the resulting value.
To avoid rolling back [14992] as a backwards incompatible change, I
propose a deprecation path.
Callables obtained from a dictionary or list index lookup can continue
to return the raw callable (along with a deprecation warning) unless
`getattr(callable, 'template_callable', False)` is `True` so that
people can opt-in to the new behaviour.
In 1.5 that changes to `getattr(callable, 'template_callable', True)`
so that the new behaviour becomes default, while still allowing people
to retain the old behaviour by setting a `template_callable` attribute
to `False` on their callable.
This should allow us to move to the new (more consistent) behaviour
and avoid the problem of a silent change in template rendering that
leaves developers with no indication as to why their previously
working templates are suddenly broken.
Another problem (described at #15025 and fixed by the patch there) is
with the debug page. It is now resolving callable stack trace frame
local variables instead of just describing them. If such a variable
raises an exception, a raw traceback is displayed in the browser and
the original traceback is lost. If it doesn't raise an exception, it's
still going to display incorrect information to the developer, e.g. a
form instance instead of a form class.
Cheers.
Tai.
Luke Plant
Thanks for your explanations and research on this. I think it is a case
where the docs don't specify the process exactly enough, so it is
somewhat borderline.
The docs don't actually make sense, as they talk about "method call" as
a "lookup", when technically there is no such thing in Python - just
attribute lookup, followed by calling a callable - and at the level of
detail we are arguing that distinction matters.
So we then have to think about what is sensible behaviour, and I think
the new behaviour is much simpler and saner, and much more consistent
with itself. Also, given the nature of the Django's template system
(i.e. usable for designers), it is difficult to come up with a use case
for the old behaviour. The use case linked by Tai is very unconvincing -
to me, using 'mycallable.func_name' is a gross thing to do in a template
- a fairly blatant case of logic that belongs in the view function.
If we decide that the old behaviour was a bug, it doesn't (technically)
matter that the fix breaks some people's templates, because our policy
is to fix bugs and not to support bugs. The only exception would be when
a large fraction of people are relying on the bug, and I don't think
that is likely to be the case here.
So, yes, you were relying on a bug IMO - Django was treating objects
retrieved by dictionary and list index lookup inconsistently compared to
objects retrieved by attribute lookup, which is not a sensible thing to
do in Python.
We may want to put a note in the backwards incompatible changes section,
as a courtesy to those whose templates may stop working. But I don't
think the deprecation path proposed by Tai would be useful - it would
just confuse things more, and it isn't appropriate if we treat the
original behaviour as a bug. I also think that mrmachine's analysis and
patch on #15025 is correct.
Thanks,
Luke
--
"Whom have I in heaven but You?
And there is none upon earth that I desire besides You." Psalm 73:25
Luke Plant || http://lukeplant.me.uk/
Tai Lee
and not having to support it while it follows a deprecation path.
However, either way, if the new behaviour stays (and I definitely
think it should) I think we should update the docs to clarify that
there was a change in behaviour in 1.3, that the new behaviour is to
try a dictionary lookup, an attribute lookup, a list index lookup, and
then to try calling the resulting object, and that the reason for the
change was for consistency (where previously Django assumed that only
object attributes could be callable).
I've created a new ticket and uploaded a patch to do just that.
http://code.djangoproject.com/ticket/15057
Luke Plant
> If we can, I would be in favour of treating the old behaviour as a bug
> and not having to support it while it follows a deprecation path.
>
> However, either way, if the new behaviour stays (and I definitely
> think it should) I think we should update the docs to clarify that
> there was a change in behaviour in 1.3, that the new behaviour is to
> try a dictionary lookup, an attribute lookup, a list index lookup, and
> then to try calling the resulting object, and that the reason for the
> change was for consistency (where previously Django assumed that only
> object attributes could be callable).
>
> I've created a new ticket and uploaded a patch to do just that.
>
> http://code.djangoproject.com/ticket/15057
Ah, you are mrmachine - thanks :-)
Ticket accepted.
Don Spaulding
On Jan 11, 8:31 pm, Luke Plant <L.Plant...@cantab.net> wrote:
>
> So we then have to think about what is sensible behaviour, and I think
> the new behaviour is much simpler and saner, and much more consistent
> with itself.
> Also, given the nature of the Django's template system
> (i.e. usable for designers), it is difficult to come up with a use case
> for the old behaviour. The use case linked by Tai is very unconvincing -
> to me, using 'mycallable.func_name' is a gross thing to do in a template
> - a fairly blatant case of logic that belongs in the view function.
into when porting to SVN trunk. I understand the "usable for
designers" mantra, and I don't pretend that 'mycallable.func_name' is
elegant, but frankly, I don't care. I don't have the luxury of
working with a dedicated designer. I write the templates. If I find
a snippet that makes my life easier in a .py file, I don't spend too
long making sure it follows the template language's philosophy.
> The only exception would be when
> a large fraction of people are relying on the bug, and I don't think
> that is likely to be the case here.
rely on the old behavior. I will say that the debug template has been
"relying on the bug" since 2005. By my estimation, that makes nearly
every template ever created a candidate for relying on the old
behavior. And for a python dev to treat callables as first-class
objects is hardly unusual.
;-)
What do you think about throwing in a nod towards backwards
compatibility by special-casing attribute access on callables?
Basically, keep the functionality of [14992] that says "Callables are
always called" and just add " unless the next lookup is for an
attribute that exists on the callable". There's a potential for name
clashes if you were expecting the template lookup to happen on the
return value of the callable instead of the callable itself, but I
would think that's a much smaller incompatibility than the one created
by [14992].
Tai Lee
On Jan 13, 5:06 am, Don Spaulding <donspauldin...@gmail.com> wrote:
>
> I can't speak to the number of templates actually in the wild that
> rely on the old behavior. I will say that the debug template has been
> "relying on the bug" since 2005. By my estimation, that makes nearly
> every template ever created a candidate for relying on the old
> behavior. And for a python dev to treat callables as first-class
> objects is hardly unusual.
>
> ;-)
normal template, it's a special case because we need to inhibit the
normal exception handling behaviour of the template system for all
exceptions. Anyone who has a similar special case view/template in
their own app should also be escaping potentially dangerous variables
before they reach the template.
> What do you think about throwing in a nod towards backwards
> compatibility by special-casing attribute access on callables?
> Basically, keep the functionality of [14992] that says "Callables are
> always called" and just add " unless the next lookup is for an
> attribute that exists on the callable". There's a potential for name
> clashes if you were expecting the template lookup to happen on the
> return value of the callable instead of the callable itself, but I
> would think that's a much smaller incompatibility than the one created
> by [14992].
ambiguous behaviour as you note. Without knowing all of the attributes
on your variables, you won't know if a template variable will resolve
to an attribute of the callable or it's return value. And when a clash
does happen, there's no easy way a template author to debug or resolve
it. I also don't think the degree of compatibility is part of the
equation. If the old behaviour is a bug (inconsistent treatment of
dict, object and list variables), we don't need to retain
compatibility. If it's not a bug, we do need to retain full backwards
compatibility.
Luke Plant
> The use case Tai linked was the first (and really, only) bug I ran
> into when porting to SVN trunk. I understand the "usable for
> designers" mantra, and I don't pretend that 'mycallable.func_name' is
> elegant, but frankly, I don't care. I don't have the luxury of
> working with a dedicated designer. I write the templates. If I find
> a snippet that makes my life easier in a .py file, I don't spend too
> long making sure it follows the template language's philosophy.
Obviously, I have no problem with you doing that. People are free to use
and abuse how they like, and to follow their own judgement as to which
is which, just as they are with Python features - and just as they are
free to do risky things like depend on internal methods and
implementation details. The philosophy of the template language is
relevant here simply for deciding which behaviour is more expected and
consistent, and working out which should be considered the bug in
borderline cases like this.
> I can't speak to the number of templates actually in the wild that
> rely on the old behavior. I will say that the debug template has been
> "relying on the bug" since 2005. By my estimation, that makes nearly
> every template ever created a candidate for relying on the old
> behavior. And for a python dev to treat callables as first-class
> objects is hardly unusual.
Yep, I'm not saying its a stupid thing to do for Python in general, I'm
saying its an unusual and unlikely choice for someone who knows that
Django's template language automatically calls callables (at least
sometimes), and knows that the language is supposed to keep view logic
out. Even though I also write my own templates in the vast majority of
cases, it would never have occurred to me to do what that snippet did,
and if you had shown me it before this discussion, I think I would have:
1) been very surprised it worked
2) quite probably classed it as a bug that needed fixing!
> What do you think about throwing in a nod towards backwards
> compatibility by special-casing attribute access on callables?
> Basically, keep the functionality of [14992] that says "Callables are
> always called" and just add " unless the next lookup is for an
> attribute that exists on the callable". There's a potential for name
> clashes if you were expecting the template lookup to happen on the
> return value of the callable instead of the callable itself, but I
> would think that's a much smaller incompatibility than the one created
> by [14992].
I agree with Tai, and think this would make the behaviour more confusing
and complicated.
Regards,
Don Spaulding
On Jan 12, 4:23 pm, Tai Lee <real.hu...@mrmachine.net> wrote:
> On Jan 13, 5:06 am, Don Spaulding <donspauldin...@gmail.com> wrote:
>
>
>
> > I will say that the debug template has been
> > "relying on the bug" since 2005. By my estimation, that makes nearly
> > every template ever created a candidate for relying on the old
> > behavior.
>
> normal template, it's a special case because we need to inhibit the
> normal exception handling behaviour of the template system for all
> exceptions.
The debug template broke one way, the snippet broke another way. Just
because nobody is out there writing another version of the debug
template, doesn't mean their templates are safe. And we're not
talking about hardcore devs who are relying on the internal
implementations of methods hidden behind Model._meta here, we're
talking about relying on a template behavior that's been around as
long as Django.
>
> > What do you think about throwing in a nod towards backwards
> > compatibility by special-casing attribute access on callables?
> > Basically, keep the functionality of [14992] that says "Callables are
> > always called" and just add " unless the next lookup is for an
> > attribute that exists on the callable". There's a potential for name
> > clashes if you were expecting the template lookup to happen on the
> > return value of the callable instead of the callable itself, but I
> > would think that's a much smaller incompatibility than the one created
> > by [14992].
>
> This feels too magical to me, and it still leaves the possibility for
> ambiguous behaviour as you note. Without knowing all of the attributes
> on your variables, you won't know if a template variable will resolve
> to an attribute of the callable or it's return value. And when a clash
> does happen, there's no easy way a template author to debug or resolve
> it.
inflicting that same pain on a different set of template authors.
Don Spaulding
On Jan 12, 6:08 pm, Don Spaulding <donspauldin...@gmail.com> wrote:
> On Jan 12, 4:23 pm, Tai Lee <real.hu...@mrmachine.net> wrote:
> > On Jan 13, 5:06 am, Don Spaulding <donspauldin...@gmail.com> wrote:
>
> > This feels too magical to me, and it still leaves the possibility for
> > ambiguous behaviour as you note. Without knowing all of the attributes
> > on your variables, you won't know if a template variable will resolve
> > to an attribute of the callable or it's return value. And when a clash
> > does happen, there's no easy way a template author to debug or resolve
> > it.
>
> It's not a great fix. I'If [14992] stays in unchanged, you're simply
> inflicting that same pain on a different set of template authors.
>
different set of template authors.
>
> > I also don't think the degree of compatibility is part of the
> > equation. If the old behaviour is a bug (inconsistent treatment of
> > dict, object and list variables), we don't need to retain
> > compatibility. If it's not a bug, we do need to retain full backwards
> > compatibility.
have a hard time believing that in the 6 years since the inception of
the template language, I'm the only one that was relying on this buggy
behavior. The snippet I was using was explicitly relying on it, the
debug page wasn't. That's a red flag to me. My guess is that [14992]
will break a lot of templates in hard-to-debug ways, but that's just
speculation.
Marco Paolini
the guys that don't want the dict-item-funcion to be called, could set
the alters_data attribute, no?
while we're at it why not using inspect module to check function
signature before calling it?
if requires an arg, raise TypeError
else call it without catching TypeError
I'm sure it has been discussed before but, hey...
Marco
Luke Plant
> +1 for [14992] and the proposed documentation change
>
> the guys that don't want the dict-item-funcion to be called, could set
> the alters_data attribute, no?
In this case, they will just get settings.TEMPLATE_STRING_IF_INVALID,
not anything useful.
> while we're at it why not using inspect module to check function
> signature before calling it?
>
> if requires an arg, raise TypeError
> else call it without catching TypeError
One reason is that is an extra cost, another is that the result of
inspect.getargspec() is still ambiguous - if you get:
ArgSpec(args=[], varargs='args', keywords='kwargs', defaults=None)
then you don't actually know whether it takes an argument or not, and
this kind of signature is often generated by decorators.
In conclusion, I'm going to leave [14992] in. Due to the backwards
incompatibility discovered, it's not a good idea to backport to 1.2.X.
But since it is cleaning up an inconsistency/ambiguity, I'm happy to
leave it in, and say that code that was relying on the old behaviour was
relying on ambiguity/inconsistency/corner case that we don't want to
support. I will add a note to the 1.3 release notes.
Thanks Don for bringing this up. None of the other core devs have
stepped up in favour of reverting or the deprecation path, so I hope
you're as happy you can be with the outcome.
Thanks,
Luke
--
"Yes, wearily I sit here, pain and misery my only companions. And
vast intelligence of course. And infinite sorrow. And..." (Marvin
the paranoid android)
Luke Plant || http://lukeplant.me.uk/
Don Spaulding
On Jan 13, 7:05 am, Luke Plant <L.Plant...@cantab.net> wrote:
>
> In conclusion, I'm going to leave [14992] in. Due to the backwards
> incompatibility discovered, it's not a good idea to backport to 1.2.X.
> But since it is cleaning up an inconsistency/ambiguity, I'm happy to
> leave it in, and say that code that was relying on the old behaviour was
> relying on ambiguity/inconsistency/corner case that we don't want to
> support. I will add a note to the 1.3 release notes.
>
> Thanks Don for bringing this up. None of the other core devs have
> stepped up in favour of reverting or the deprecation path, so I hope
> you're as happy you can be with the outcome.
Thanks for taking the time to look into this in detail.
bpeschier
more consistent. The snag I run into was a flexible filter for making
ModelForm-instances which took a form-class as an argument via a
Variable. Since classes are callable (their constructor), suddenly I
had a bunch of instances instead of classes. I am currently reworking
that flow, but I have to admit I had to think twice before it dawned
on me.
Marco Paolini
what if you add an alters_data class attribute to your ModelForm subclass?
class MyForm(ModelForm):
alters_data = True
....
this way __call__ is not going to get called...
bpeschier
> class MyForm(ModelForm):
> alters_data = True
> ....
>
> this way __call__ is not going to get called...
See: http://code.djangoproject.com/browser/django/trunk/django/template/base.py#L694
Marco Paolini
then maybe a new special attribute like "dont_call_me" or
"i_will_call_you_if_needed" (way more polite)
could be handy in this situations? What do you think?
Marco
Łukasz Rekucki
>
> then maybe a new special attribute like "dont_call_me" or
> "i_will_call_you_if_needed" (way more polite)
>
> could be handy in this situations? What do you think?
I think it's too much of a corner case to justify another check in lookup.
When I need to pass a class to a tag (which is quite rare), I prefer
to use it's name (e.g. "app.forms.MyForm") and fetch the actuall
object later. If you really need a to pass the callable itself, you
can wrap it in a 1-tuple or a custom proxy object.
--
Łukasz Rekucki