Skip to content

Make the translation sanitization optional - #20976

Merged
ChristophWurst merged 2 commits into
masterfrom
fix/make-translation-sanitization-optional
May 27, 2020
Merged

Make the translation sanitization optional#20976
ChristophWurst merged 2 commits into
masterfrom
fix/make-translation-sanitization-optional

Conversation

@ChristophWurst

@ChristophWurstChristophWurst commented May 14, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#18662

Ref #18741

@ChristophWurstChristophWurst added the 3. to review Waiting for reviews label May 14, 2020
@ChristophWurstChristophWurst added this to the Nextcloud 20 milestone May 14, 2020
@ChristophWurstChristophWurst self-assigned this May 14, 2020
@ChristophWurst
ChristophWurstforce-pushed the fix/make-translation-sanitization-optional branch from 942aa1f to 73aa1f2CompareMay 14, 2020 13:38
georgehrke
georgehrke requested changes May 14, 2020
View reviewed changes

@georgehrkegeorgehrke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not entirely sure about this approach, because it requires you to add {}, undefined, {sanitize: false} to all translations.

Devices & Sessions is a case where we know it will break, but there are many other cases where the english source version will work fine, but issues will only occur in specific languages for certain strings.

I think that we should keep the current t for legacy reasons and introduce a new translateUnescaped, which we can use whenever we are inside a templating system like handlebars or vue that takes care of escaping for us.

@ChristophWurst

ChristophWurst commented May 14, 2020

Copy link
Copy Markdown
MemberAuthor

I think that we should keep the current t for legacy reasons and introduce a new translateUnescaped, which we can use whenever we are inside a templating system like handlebars or vue that takes care of escaping for us.

Shoot a PR that fixes is and is backportable and I'll close mine.

This solution is what @nickvergessen, @rullzer and I discussed and settled on.

@georgehrke
georgehrke self-requested a review May 14, 2020 14:43
@georgehrke

georgehrke commented May 14, 2020

Copy link
Copy Markdown
Member

This solution is what @nickvergessen, @rullzer and I discussed and settled on.

Withdrawing my review in that case. Fine by me.

@jumpswitch

jumpswitch commented May 15, 2020

Copy link
Copy Markdown

I just made dirty workaround until this isn`t merged.
I replaced all occurences of 'Devices & settings' to 'Devices and settings' in apps/settings/js/vue-settings-personal-security.js*

@ChristophWurst
Make the translation sanitization optional
74db919
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Fix devices & sessions sanitization
069d02a
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
ChristophWurstforce-pushed the fix/make-translation-sanitization-optional branch from 73aa1f2 to 069d02aCompareMay 27, 2020 13:09
@ChristophWurst

ChristophWurst commented May 27, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

@ChristophWurst

ChristophWurst commented May 27, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable18

rullzer
rullzer approved these changes May 27, 2020
View reviewed changes

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm still a fan. But yeah I guess so :S

@ChristophWurst

ChristophWurst commented May 27, 2020

Copy link
Copy Markdown
MemberAuthor

I'm still a fan. But yeah I guess so :S

We can fix it properly once we have a consensus on what a proper solution is.

@ChristophWurst
ChristophWurst merged commit 052ddc3 into masterMay 27, 2020
@ChristophWurst
ChristophWurst deleted the fix/make-translation-sanitization-optional branch May 27, 2020 13:47
@backportbot-nextcloud

backportbot-nextcloudBot commented May 27, 2020

Copy link
Copy Markdown

The backport to stable19 failed. Please do this backport manually.

@backportbot-nextcloud

backportbot-nextcloudBot commented May 27, 2020

Copy link
Copy Markdown

The backport to stable18 failed. Please do this backport manually.

This was referenced May 27, 2020
Merged
Merged
@nickvergessennickvergessen mentioned this pull request May 28, 2020
Merged
@raimund-schluesslerraimund-schluessler mentioned this pull request Jul 14, 2020
Merged
@raimund-schluesslerraimund-schluessler mentioned this pull request Jul 29, 2020
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzerrullzerrullzer approved these changes
@nickvergessennickvergessennickvergessen approved these changes
@juliusknorrjuliusknorrAwaiting requested review from juliusknorr
@georgehrkegeorgehrkeAwaiting requested review from georgehrke

Assignees

@ChristophWurstChristophWurst

Labels

3. to reviewWaiting for reviews

Projects

None yet

Milestone

Nextcloud 20

Development

Successfully merging this pull request may close these issues.

Escaping of Ampersand "&" in translation strings broken

6 participants

@ChristophWurst@georgehrke@jumpswitch@rullzer@nickvergessen@MorrisJobke