Fixed #19088 - Always escape % inside blocktrans tag. #442

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

lqc commented Oct 13, 2012

No description provided.

django/templatetags/i18n.py
+ # avoid infinite recursion
+ if settings.DEBUG:
+ raise TemplateSyntaxError("'blocktrans' is unable to format string returned by gettext: '%r' using %r" % result, data)
+ return ''
@claudep

claudep Oct 18, 2012

Member

Two things to note here:

  1. I don't like much the dependence on settings.DEBUG. Either unconditionally raise, as done in other parts of this file, or make usage of the logging infrastructure (getLogger('django.template'), logger.error(...)).
  2. I'd like to keep the return statement at the end, if possible
@lqc

lqc Oct 18, 2012

Ad 1. Most of builtin nodes refer to either settings.DEBUG or settings.TEMPLATE_DEBUG for raising exceptions, none use logging. I don't see a reason to treat this error differently than any other runtime error in templates.

Ad 2. Ok.

@claudep

claudep Oct 18, 2012

Member

I know that logging is currently not used, but it might/should in the future. Let us forget this now, this should be the subject of a separate ticket anyway. But then I'd advocate for unconditionally raising the TemplateSyntaxError.

tests/regressiontests/i18n/tests.py
+ t = Template('{% load i18n %}{% blocktrans %}My name is {{ person }}.{% endblocktrans %}')
+ with self.assertRaisesRegexp(TemplateSyntaxError, "'blocktrans' is unable to format string returned by gettext: .*"):
+ t.render(Context({}))
+
@claudep

claudep Oct 22, 2012

Member

Interestingly (or not), this test is not causing infinite recursion before the patch. It simply gives untranslated string, without placeholder ("My name is ."). After the patch, it raises TemplateSyntaxError, indeed. Is this intended? Can you just have a look at this behaviour?

@lqc

lqc Oct 22, 2012

I think I misunderstood what override(None) really does. Apparently it switched gettext to NullTranslation, but for some reason get_language() returns settings.LANGUAGE_CODE, which is a bit misleading. I thought it just switched translation to settings.LANGUAGE_CODE, my bad.

Restored the original behavior in updated diff, although I'm not 100% convinced that returning untranslated string is the best thing to do here.

+ t = Template('{% load i18n %}{% blocktrans %}My name is {{ person }}.{% endblocktrans %}')
+ rendered = t.render(Context({'person': 'James'}))
+ self.assertEqual(rendered, 'My name is James.')
+
@claudep

claudep Oct 23, 2012

Member

This test doesn't fail either in current code. I suggest to simply drop it. No need to update the pull request, I'm going to commit the patch soon.

@lqc

lqc Oct 23, 2012

I know it passes on current code. I included it to show it also passes on changed code, to make sure there is no regression in this case (as the previous patch did). Thanks for reviewing this and sorry for the trouble.

Member

claudep commented Oct 23, 2012

Thanks, committed in 9fd2f9c

@claudep claudep closed this Oct 23, 2012

nanuxbe pushed a commit to nanuxbe/django that referenced this pull request Jul 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment