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

Use placeholders for URLs in translation strings #6507

Merged
merged 2 commits into from Nov 12, 2023

Conversation

leofeyer
Copy link
Member

So that we can change the URLs without creating unnecessary work for the translators. See contao/to.contao.org#80 for more information.

@leofeyer leofeyer added the bug label Nov 10, 2023
@leofeyer leofeyer added this to the 4.13 milestone Nov 10, 2023
@leofeyer leofeyer self-assigned this Nov 10, 2023
@leofeyer leofeyer requested review from Toflar and a team November 10, 2023 15:09
@leofeyer
Copy link
Member Author

On second thought, passing the replacement strings to the trans() method ties the template to our translator. This way, the template is no longer compatible with the default Twig parser, which it would be if we continued to use format().

@contao/developers What do you think? Do we care?

@leofeyer leofeyer added the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Nov 10, 2023
@Toflar
Copy link
Member

Toflar commented Nov 10, 2023

I don't understand the issue. You want to translate a string, this always ties it to a translator? And what do you mean by "default Twig parser"? trans is a filter added by the fullstack framework of Symfony (or the Bridge more specifically) so it's already tied to the framework and it's not a Twig standalone filter at all.
Or in other words, if that were an issue, we were also not allowed to decorate the translator and use it the way we are already using it in many places (like here for example).

@fritzmg
Copy link
Contributor

fritzmg commented Nov 10, 2023

The template is part of contao/core-bundle and contao/core-bundle decorates the Translator - thus there should never be an issue.

@leofeyer
Copy link
Member Author

Ok, then I will merge this as soon as it has been approved.

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

LGTM but I think we should still ask @m-vo for a review just because it's affecting Twig and I don't want to interfere with his plans here.

@leofeyer
Copy link
Member Author

Agreed, although none of these templates are related to his work. This is Contao 4.13 after all. 😄

@leofeyer leofeyer removed the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Nov 10, 2023
@m-vo
Copy link
Member

m-vo commented Nov 11, 2023

Changes look good to me.

There is one thing we could think about, though: With HTML in the translated strings, we need to bypass the escaper (|raw). But ideally, we would encode anything but the link, which we could totally do now in Contao 5 with insert tags and the insert_tag_raw filter:

<trans-unit id="XPT.tokenExplainTwo">
    <source>Learn more about speeding up your workflow by using {{link_open::%s::blank}}keyboard shortcuts{{link_close}}.</source>
</trans-unit>
<p>{{ 'XPT.tokenExplainTwo'|trans(['https://to.contao.org/support'])|insert_tag_raw }}</p>

Wdyt?

@ausi
Copy link
Member

ausi commented Nov 12, 2023

I don’t like the idea of using insert tags, especially things like {{link_open::*}} are problematic IMO.

I think the “correct” solution would be that the link comes from the templates like this (pseudocode):

<trans-unit id="message">
    <source>Learn more about speeding up your workflow by using [keyboard shortcuts].</source>
</trans-unit>
{{ 'message'|trans({'[' => '<a href="…">', ']' => '</a>'}) }}

Not sure if this can be implemented in a way so that the message still gets encoded correctly.

But for now I think we can keep it as it is.

@leofeyer leofeyer merged commit ac1fc26 into contao:4.13 Nov 12, 2023
17 checks passed
@leofeyer leofeyer deleted the fix/shortcuts branch November 12, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants