fix template label nested translation (#6687) #6896
Conversation
|
any feedback on this @Aiky30 or @victor-yunenko |
0d249bf to
cb423f9
Compare
There was a problem hiding this comment.
I think that these changes fix when I18n is disabled but then break the scenario for the majority when it is enabled because the UI item will no longer be translated for users using a language other than English.
I think that building the template list should be different dependant on the I18N Setting. This then protects both use cases.
|
When should the labels be translated? The docs indicate, by example (http://docs.django-cms.org/en/latest/reference/configuration.html#cms-templates), that they should be translated when configured. In that case, my fix is correct for both cases. If instead sites in all languages should configure the labels untranslated, then you are right. There is an alternative fix: storing the TEMPLATE_INHERITANCE_MAGIC label untranslated. That works only because nested gettext doesn't break with I18N turned on. But it's the least disruptive. If you'd prefer that, would you like me to change the docs, too, or leave them as-is? John |
It was the template inheritance text that I thought would have then been broken but you're right that it's stored with translations active which we should keep: https://github.com/divio/django-cms/blob/c4b48fab0f84c237b436442039137ac1921e2093/cms/utils/conf.py#L168 I was thinking of a simple if else condition but you're right that it is covered already so no action needed, the test looks sane. I'm happy. My apologies for the confusion. Andrew |
|
@jbazik Can you bring the branch back up to date so that we can see the build pass. Once passed I will re-review and accept. If you could add a changelog entry whilst you are making changes that would help too please. |
9776797 to
c980a00
Compare
|
I added a changelog entry and rebased. |
There was a problem hiding this comment.
I'm happy with this change.
|
@FinalAngel I'm happy with the change. |
|
I am happy with these changes |
|
👍 will merge once the tests pass |
|
@jbazik Looks like this might benefit from another merge of develop to pick up the differences in the coverage and coveralls requirements. |
48d6964 to
a10f6ca
Compare
|
@marksweb done - thanks |
|
@FinalAngel This is now passing after being updated |
There was a problem hiding this comment.
Due to the nature of this change i.e. anyone who didn't follow the docs on translations can we please add a changelog entry, this will most likely make it into django-cms 3.9.x and could be considered a breaking change. My apologies, we have recently agreed that no changes should be coming in without a changelog entry, especially if there are potentially breaking changes. I'm sure you would appreciate this if the change affected your project.
I will personally take responsibility for getting the change merged ASAP with a changelog entry in place.
Thank you.
a10f6ca to
2970cc2
Compare
|
@Aiky30 added changelog entry, thanks. |
There was a problem hiding this comment.
Happy with the changes, just needs the tests to pass now.
ca7c387 to
e1a5946
Compare
|
Rebased against develop again. |
|
@victor-yunenko This PR requires a review from you to get wrapped up. |
Fixes a serious bug in 3.8.0 to that causes an exception in the admin when editing any page, when USE_I18N is set to False. Fixes django-cms#6687 Details: django-cms#6896 (comment)
|
This happens for me in v5.0.0 has it recurred? |
|
@sparrowme Interestingly, it has. :-( |
|
@sparrowme Where exactly did you encounter the issue? |
Description
Fixes a serious bug in (at least) 3.7.2 to dev that causes an exception in the admin when editing any page, when USE_I18N is set to False. Fixes #6687.
In utils/conf.get_templates, the inherit template label is stored as a gettext_lazy object:
In models/pagemodel.py, all template label choices are stored as gettext_lazy objects:
This creates a nested lazy object, which results in an exception in django.template.base.render_value_in_context when it is coerced to a string:
This only happens when USE_I18N is False. In that case, the function stored in the lazy object is django.utils.translation.gettext_noop, which just returns it's argument. The str() constructor doesn't know what to do with the stored proxy object.
The docs suggest that CMS_TEMPLATES should contain translated labels, so the correct fix is to not apply translation when building the choices list in the Page model.
A test is included. Note that it is not enough to
override_settings(USE_I18N=False)when testing, since the translation mechanism in django is set up during initialization and is not affected by the override. My test includes a mock to test against gettext_noop.Related resources
Checklist
develop