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

Some translations might be displayed with encoded entities #19190

Merged
merged 10 commits into from Sep 14, 2022

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented May 6, 2022

Description:

The were various translations including a & or >. While this in general is fine, we currently have the problem that various translations of those strings now include a & or > instead, as weblate automatically escapes them due to the active safe_html filter. This can currently cause that the escaped chars are displayed in other languages.

I have now converted all & to & in the english sources and updated all usages of those strings so they are used 'raw', to avoid double encoding.

@diosmosis I've also updated ContentBlock.vue so the title is included as v-html, can you quickly check if it's fine that way?

Review

@sgiehl sgiehl added the Needs Review PRs that need a code review label May 6, 2022
@sgiehl sgiehl added this to the 4.11.0 milestone May 6, 2022
@diosmosis
Copy link
Member

Wouldn't it be better to just not escape them in weblate so they are escaped in twig/vue?

@sgiehl
Copy link
Member Author

sgiehl commented May 8, 2022

@diosmosis I guess we would need to disable the safe_html filter on weblate for that, which I would prefer not to do.
Imho we actually should in general "trust" translations not to contain bad html and use them raw & unescaped.

@diosmosis
Copy link
Member

My personal opinion is that with a framework like Vue where it is, explicitly in the documentation, discouraged to ever use v-html, increased uses should be avoided. And it is much easier to simply assume all template text is unsafe and use innerText/textContent (indirectly by vue) than it is to have different classes of input where some is actually unsafe and some is maybe safe, and have to consciously remember them.

That said I am not in charge of this so will defer approval of the PR to someone else.

The TypeScript/Vue related changes look fine.

@sgiehl
Copy link
Member Author

sgiehl commented May 9, 2022

@Findus23 As you are also involved in the translations stuff. What is your opinion on that one?

@justinvelluppillai
Copy link
Contributor

Happy for you to make the decision here @sgiehl

@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label May 20, 2022
@justinvelluppillai justinvelluppillai removed Needs Review PRs that need a code review Stale The label used by the Close Stale Issues action labels May 26, 2022
@justinvelluppillai justinvelluppillai modified the milestones: 4.11.0, 4.12.0 Jun 7, 2022
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 22, 2022
@Findus23
Copy link
Member

I didn't see this PR yet, but have to say that I don't have a strong opinion either way. Not using v-html (or raw) feels cleaner to me and should leave a lot less mental overhead in the whole process. But of course one needs to keep in mind that it is always possible for a translation to contain "special characters" that the English source string doesn't contain, so accidentally encoding twice can cause things like #19626

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Aug 17, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Sep 1, 2022
@sgiehl sgiehl merged commit 253bae5 into 4.x-dev Sep 14, 2022
@sgiehl sgiehl deleted the translationfixes branch September 14, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants