Reallow the {% for %} tag to unpack any iterable

[Sergei Maertens]

In 1.10 there are now checks done before unpacking in a for loop:

{% for foo, bar in items %}
    ...
{% endfor %}

Where it is verified that everey item in items has len(item) == 2 in this particular case.

However, the check is implemented so that len(item) is taken only if the item is an instance of tuple or list, while custom objects may also implement def __len__(self):. Because the custom object does not subclass from tuple or list, it's length is set to 1, leading to ValueErrors.

I have a working patch at ​https://github.com/django/django/compare/master...sergei-maertens:duck-type-forloop-unpack-len?expand=1, tests are passing. The check no longer checks if the item is an instance of tuple/list, but just tries to get the len(item) and handles TypeErrors.

The only case I could imagine was where the items would be an iterable of strings, but even then it turns out the check is correct (as it is technically possible to unpack strings).

[Claude Paroz]

+1 for one less isinstance if possible. The tricky thing might be to handle strings with backwards compatibility.

Did you spot a regression in 1.10?

[Tim Graham]

The ticket for the deprecation and error message is #13408. That code isn't new in 1.10 (rather in 1.8) so there's no regression as far as I understand. I removed the code comment about "To complete this deprecation" earlier today (ba749f8f87dd60b20aeaefb84ee182f192746ebf) as that deprecation did complete in 1.10.

Here's a test case that passes on master but fails with your proposed patch. As to whether or not that's a case to protect against, I guess it's probably better to assume that the programmer won't make a mistake like that if it prevents other use cases.

@setup({'for-tag-unpack15': '{% for x,y in items %}{{ x }}:{{ y }}/{% endfor %}'})
def test_for_tag_unpack15(self):
    with self.assertRaisesMessage(ValueError, 'Need 2 values to unpack in for loop; got 1.'):
        self.engine.render_to_string('for-tag-unpack15', {'items': ('ab', 'ac')})

[Sergei Maertens]

I'm not sure that I completely understand - are you saying this protection was introduced in 1.8 already? Because that would surprise me, since the package tested against Django 1.7, 1.8 and 1.9 before and only started failing against 1.10.

[Sergei Maertens]

Replying to timgraham:

Here's a test case that passes on master but fails with your proposed patch. As to whether or not that's a case to protect against, I guess it's probably better to assume that the programmer won't make a mistake like that if it prevents other use cases.

@setup({'for-tag-unpack15': '{% for x,y in items %}{{ x }}:{{ y }}/{% endfor %}'})
def test_for_tag_unpack15(self):
    with self.assertRaisesMessage(ValueError, 'Need 2 values to unpack in for loop; got 1.'):
        self.engine.render_to_string('for-tag-unpack15', {'items': ('ab', 'ac')})

Ah yes, this was the test case I was trying to come up with because I _knew_ there was something with checking string lengths. I think I would classify this test case as incorrect, since in pure python for x, y in ('ab', 'ac'): is correct as well. I'd call this a logical programmer error that's the responsability of the end-user.

[Tim Graham]

My understanding is that the change in 1.10 was to convert a deprecation warning to an exception: 3bbebd06adc36f31877a9c0af6c20c5b5a71a900. Maybe you missing those warnings in earlier versions? Or else the issue is something different, in which case, could you provide a test case and bisect to find the commit where the behavior changed?

[Tim Graham]

​PR. Even if this did raise warnings in older versions, I guess it should be backported to 1.10 if the ValueError is now prohibiting the use case.

[GitHub <noreply@…>]

In 937d752d:

Fixed #27058 -- Reallowed the {% for %} tag to unpack any iterable.

Thanks Sergei Maertens for the report and patch.

[Tim Graham <timograham@…>]

In 020ba4b:

[1.10.x] Fixed #27058 -- Reallowed the {% for %} tag to unpack any iterable.

Thanks Sergei Maertens for the report and patch.

Backport of 937d752d3deabebe60dfbe9ff9823772730f336a from master