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

Fix untranslatable strings #1371

Merged
merged 14 commits into from
Jul 3, 2022
Merged

Fix untranslatable strings #1371

merged 14 commits into from
Jul 3, 2022

Conversation

qwerty287
Copy link
Contributor

@qwerty287 qwerty287 commented Jun 13, 2022

  • allow translation of untranslatable strings
  • remove some duplications
  • fix and improve some German strings
  • Never concat strings: Use a template system (%s, %d...) instead of just writing multiple strings behind each other

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1371 (b35a64a) into master (e9d5693) will decrease coverage by 0.81%.
The diff coverage is n/a.

@ildyria
Copy link
Member

ildyria commented Jun 13, 2022

Never concat strings: Use a template system ( %0, %1...) instead of just writing multiple strings behind each other

I like the idea and intent, but I am not sure of the execution as we will also be using those strings in Php.
I would rather use sprintf format (%s etc) and make use of e.g. https://www.npmjs.com/package/sprintf-js

@qwerty287
Copy link
Contributor Author

qwerty287 commented Jun 13, 2022

Does PHP has a native template system (or a library similar to the JS lib you linked)? Because the current system supports PHP as well using str_replace. Found it, https://www.php.net/manual/de/function.sprintf.php.

@LycheeOrg LycheeOrg deleted a comment from codecov bot Jun 13, 2022
@qwerty287
Copy link
Contributor Author

@ildyria Updated it in a12bf2a.

@nagmat84
Copy link
Collaborator

I haven't had a look at it. I like the idea and it isa step in the right direction. It is an improvement in any case compared to what has been there before.

In the long run (not this PR) we should try to replace sprintf by a proper translation engine which also supports multiple plural and inflections. For example in the pattern 'Do you want to delete %d photo(s)', the object "photo" must be in singular for "1" but in plural form for "0" and any number greater than one. Different rules hold for other languages. (I believe Russian has different forms for one, two and anything else.) Symfony (my framework of choice when in comes to PHP) has a great engine which supports nearly all languages, provides replacement for named place holders (i.e. something like 'Do you want to delete %number_of_photos% photo(s)') and correctly handles plural forms and inflections. I hope/assume that Laravel provides something similar.

@qwerty287
Copy link
Contributor Author

Our core issue is that we always need to work with two platforms: PHP and JS. We need easy support for both. If we fully switch over to Livewire, this is easier I believe. With a quick internet search I found https://www.codeandweb.com/babeledit/tutorials/how-to-translate-your-laravel-project which looks promising. It supports pluralization etc. in any way (yes, the Russian way is supported as well). This issue probably needs to wait for a full-livewire frontend.

Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

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

PHP side LGTM.

@ildyria
Copy link
Member

ildyria commented Jul 2, 2022

I would love to merge this soon, but I think we should wait for #1382

@ildyria
Copy link
Member

ildyria commented Jul 3, 2022

I see that you are using %1$s and %2$s. Unfortunately those are not compatible with php sprintf. :(

echo sprintf("Weet je zeker dat je dit album wilt samenvoegen '%1$s' met het album '%2$s'?", 'test', 'test');

Leads to:

NOTICE Undefined variable: s on line number 3
NOTICE Undefined variable: s on line number 3
Weet je zeker dat je dit album wilt samenvoegen ' met het album '?

The reason being that $s is being interpolated from php variables.
I strongly believe this should be :

echo sprintf("Weet je zeker dat je dit album wilt samenvoegen '%s' met het album '%s'?", 'test', 'test');

Weet je zeker dat je dit album wilt samenvoegen 'test' met het album 'test'?

@qwerty287
Copy link
Contributor Author

While you're right, this doesn't affect us, I believe. The $s is part of the string and not parsed as variables. The reason is that we use ' instead of " which does not support variable interpolation.
If you run:

echo sprintf('Weet je zeker dat je dit album wilt samenvoegen \'%1$s\' met het album \'%2$s\'?', 'test', 'test');

the output is:

Weet je zeker dat je dit album wilt samenvoegen 'test' met het album 'test'?

That's exactly how we are doing it.

@ildyria
Copy link
Member

ildyria commented Jul 3, 2022

Thanks, TIL. :)

Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

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

PHP & JS side LGTM & tested.

@qwerty287 qwerty287 merged commit 39ceb02 into master Jul 3, 2022
@qwerty287 qwerty287 deleted the fix-translations branch July 3, 2022 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants