Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed error caused by the fallback implementation of gettext #2586

Merged

Conversation

LeXofLeviafan
Copy link
Contributor

Turns out that when used with keywords in templates, gettext() gets invoked by Jinja without keyword params (guess they're applied to the returned string instead). Thus, the current fallback implementation causes an error in such a case.

@@ -4,11 +4,11 @@
except ImportError:

def gettext(string, **variables):
return string % variables
return string if not variables else string % variables
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is based directly on flask-babel implementation.

@samuelhwilliams
Copy link
Contributor

Do you have a reproduction example or a test case we could add?

@LeXofLeviafan
Copy link
Contributor Author

Server code during app init (to enable app-level translations in templates):

app.jinja_env.add_extension('jinja2.ext.i18n')
app.jinja_env.install_gettext_callables(gettext, ngettext, newstyle=True)

If the gettext passed into this is the fallback implementation from flask-admin (e.g. when flask-babel is not installed within the env), a template expression like this would raise a KeyError: 'bar' at page render:

{{ _('foo %(bar)s baz', bar='<code>bar</code>'|safe) }}  {# usecase: injecting HTML into text #}

As mentioned in the PR description, trying to debug the error reveals that when gettext() is invoked, it receives the translation string but no keywords.

Copy link
Contributor

@samuelhwilliams samuelhwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this locally for functionality and it does seem correct - nice catch.

@samuelhwilliams samuelhwilliams merged commit d9fffcf into pallets-eco:master Jan 1, 2025
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants