Opened 8 years ago

Closed 7 years ago

#4539 closed (fixed)

translation.ugettext loses variable content from blocktrans

Reported by: permonik@… Owned by: permon
Component: Internationalization Version: master
Severity: Keywords: blocktrans sprintsept14
Cc: permonik@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Look at the following template:

{% load i18n %}
{% for spell in spells %}
    {% blocktrans with spell.spell as spell and spell.wave as wave and spell.target as target and spell.caster as caster and spell.count as count%}
        '{{spell}}', '{{wave}}', '{{target}}', '{{caster}}', '{{count}}'
    {% endblocktrans %}
{% endfor %}

Imagine corresponding correct input list of dictionaries (spells). Output looks like this:

'spell', '', '', 'caster', 'count'

So, variables target and wave gets lost. This behaviour heavily depends on other variables, e.g. blocktrans with only wave and target works fine.

Behaviour could be seen in actual SVN version (rev. 5465). Simplest view and template are attached. Feel free to ask any questions, I'll be monitoring this ticket.

Attachments (4)

testing.html (300 bytes) - added by permonik@… 8 years ago.
Template used in example
views.py (257 bytes) - added by permonik@… 8 years ago.
Sample view
patch.diff (639 bytes) - added by permon 8 years ago.
patch
patch.2.diff (798 bytes) - added by permon 8 years ago.
patch - updated

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by permonik@…

Template used in example

Changed 8 years ago by permonik@…

Sample view

comment:1 Changed 8 years ago by Simon G. <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 follow-up: Changed 8 years ago by Simon G. <dev@…>

  • Summary changed from blocktrans 'losing' variable content to translation.ugettext loses variable content from blocktrans

I can confirm this behavior on r5724. It appears that the values are getting lost when going through translation.ugettext in BlockTranslateNode.render

Changed 8 years ago by permon

patch

Changed 8 years ago by permon

patch - updated

comment:3 in reply to: ↑ 2 Changed 8 years ago by permonik@…

  • Has patch set

Replying to Simon G. <dev@simon.net.nz>:

I can confirm this behavior on r5724. It appears that the values are getting lost when going through translation.ugettext in BlockTranslateNode.render

Problem solved. Context was updated during variable resolving. So, in rare cases, could occur resolving from bad scope. Small patch adds temporary dictionary, which is after successful resolving put to context as inner scope. Please, check, that I understand well to how Context.update() works. It looks, that it doesn't update current scope but adds new one. So I've worked with it in this way.
Patch is against r6098

comment:4 follow-up: Changed 8 years ago by mtredinnick

This doesn't look like the right solution. context.push() doesn't clean out the context; it only adds a marker, in effect, so that we can remove (pop) any new content added since that point. The existing content is still accessible.

So this patch doesn't look like it's getting to the heart of the problem, which is why the variables are resolving to blank in the first place if they exist in the context.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by permonik@…

Replying to mtredinnick:

This doesn't look like the right solution. context.push() doesn't clean out the context; it only adds a marker, in effect, so that we can remove (pop) any new content added since that point. The existing content is still accessible.

So this patch doesn't look like it's getting to the heart of the problem, which is why the variables are resolving to blank in the first place if they exist in the context.

Look at the example:
first is created context for loop:
[{spell: {spell: 'spell', wave: 'wave', target: 'target', caster: 'caster' count: 'count'}}
then we move into blocktrans and try to resolve variable:

  • spell.count is resolved as count and pushed to the context
  • spell.caster is resolved as caster and pushed to the context
  • spell.spell is resolved as 'spell' and pushed to the context
  • this is crucial point - from now spell.* takes resolution from wrong (new) context level, so:
  • spell.target is wrong (spell.spell.target doesn't exist) and results to
  • spell.wave is wrong (spell.spell.wave doesn't exist) and results to

context corresponding to these step is as follows:

  • [{u'count': u'count'}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
  • [{u'count': u'count', u'caster': u'caster'}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
  • [{u'count': u'count', u'caster': u'caster', u'spell': u'spell'}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
  • [{u'count': u'count', u'caster': u'caster', u'spell': u'spell', u'target': }, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
  • [{u'count': u'count', u'caster': u'caster', u'spell': u'spell', u'target': , u'wave': }, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]

If context[0] doesn't exist during computation of new context level, everything goes ok. So I still think, that my patch is correct solution. We cannot resolve variable from context which is unde generation. We have to use upper levels (so context[1] and upper levels). So I still think, that my patch is correct solution.

comment:6 in reply to: ↑ 5 Changed 8 years ago by anonymous

It would be better seen as code:

[{u'count': u'count'}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
[{u'count': u'count', u'caster': u'caster'}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
[{u'count': u'count', u'caster': u'caster', u'spell': u'spell'}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
[{u'count': u'count', u'caster': u'caster', u'spell': u'spell', u'target': ''}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]
[{u'count': u'count', u'caster': u'caster', u'spell': u'spell', u'target': '', u'wave': ''}, {'forloop': {'parentloop': {}, 'last': True, 'counter': 1, 'revcounter0': 0, 'revcounter': 1, 'counter0': 0, 'first': True}, u'spell': {'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}}, {'spells': [{'count': 'count', 'caster': 'caster', 'spell': 'spell', 'target': 'target', 'wave': 'wave'}]}]

P.S. I look at the last sentence and it looks little bit crude. I didn't think it, only cut & paste in wrong place ;-)

comment:7 Changed 8 years ago by permon

  • Owner changed from nobody to permon
  • Status changed from new to assigned

comment:8 Changed 8 years ago by permon

  • Keywords sprintsept14 added

comment:9 Changed 8 years ago by permon

  • Triage Stage changed from Accepted to Ready for checkin

comment:10 Changed 7 years ago by mtredinnick

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

(In [7261]) Fixed #4539 -- Fixed a subtle context resolving bug in the i18n template tag.
Excellent debugging from permonik@….

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