SMF
SMF copied to clipboard
SimpleMachines
Needs a string literal, not a boolean, to work properly in CrowdIn.
Fixes #7537
Note that before the change, the string is not even available for translation at all. Thus, all forums are stuck in "ltr".
BEFORE screenshots - the field doesn't appear for translation at all:
AFTER screenshots. The field is now available for translation (it's the "0"). Conducting an end-to-end test, exporting the language from CI & importing it into a test forum, you can see the theme properly displaying as rtl for Arabic:
This shows the setting translated in 2 languages in the test CrowdIn environment:
And this is upon loading the language into SMF:
./other/install.php:1945:<html', $txt['lang_rtl'] == true ? ' dir="rtl"' : '', '>
./other/install.php:1952: ', $txt['lang_rtl'] == true ? '<link rel="stylesheet" href="Themes/default/css/rtl.css">' : '', '
./other/upgrade.php:3702: content.write(\'<html', $txt['lang_rtl'] == true ? ' dir="rtl"' : '', '>\n\t<head>\n\t\t<meta name="robots" content="noindex">\n\t\t\');
./other/upgrade.php:3783:<html', $txt['lang_rtl'] == true ? ' dir="rtl"' : '', '>
./other/upgrade.php:3790: ', $txt['lang_rtl'] == true ? '<link rel="stylesheet" href="' . $settings['default_theme_url'] . '/css/rtl.css">' : '', '
./Sources/Load.php:2506: $context['right_to_left'] = !empty($txt['lang_rtl']);
./Sources/ScheduledTasks.php:1047: $context['right_to_left'] = !empty($txt['lang_rtl']);
./Sources/ManageLanguages.php:994: $primary_settings = array('native_name' => 'string', 'lang_character_set' => 'string', 'lang_locale' => 'string', 'lang_rtl' => 'bool', 'lang_dictionary' => 'string', 'lang_recaptcha' => 'string');
./Themes/default/languages/index.english.php:19:$txt['lang_rtl'] = false;
./Themes/default/languages/Install.english.php:6:$txt['lang_rtl'] = false;
Did we make sure the checks work to do the dir and the css file as expected? We could cleanup the checks and update all the checks to use $context['right_to_left'] instead.
The ManageLanguages.php change needs fixed as we have a built in language editor.
PHP does type juggling for loose comparisons, which those are.
On Mon, Aug 15, 2022 at 11:56 AM Jeremy D @.***> wrote:
./other/install.php:1945:<html', $txt['lang_rtl'] == true ? ' dir="rtl"' : '', '> ./other/install.php:1952: ', $txt['lang_rtl'] == true ? '' : '', ' ./other/upgrade.php:3702: content.write('<html', $txt['lang_rtl'] == true ? ' dir="rtl"' : '', '>\n\t
\n\t\t\n\t\t'); ./other/upgrade.php:3783:<html', $txt['lang_rtl'] == true ? ' dir="rtl"' : '', '> ./other/upgrade.php:3790: ', $txt['lang_rtl'] == true ? '' : '', ' ./Sources/Load.php:2506: $context['right_to_left'] = !empty($txt['lang_rtl']); ./Sources/ScheduledTasks.php:1047: $context['right_to_left'] = !empty($txt['lang_rtl']); ./Sources/ManageLanguages.php:994: $primary_settings = array('native_name' => 'string', 'lang_character_set' => 'string', 'lang_locale' => 'string', 'lang_rtl' => 'bool', 'lang_dictionary' => 'string', 'lang_recaptcha' => 'string'); ./Themes/default/languages/index.english.php:19:$txt['lang_rtl'] = false; ./Themes/default/languages/Install.english.php:6:$txt['lang_rtl'] = false;Did we make sure the checks work to do the dir and the css file as expected? We could cleanup the checks and update all the checks to use $context['right_to_left'] instead.
The ManageLanguages.php change needs fixed as we have a built in language editor.
— Reply to this email directly, view it on GitHub https://github.com/SimpleMachines/SMF/pull/7538#issuecomment-1215612294, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADJNN63JZX3IMMBX5DENSDVZKHHDANCNFSM56S4C4WQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
in load.php the var $context['right_to_left'] gets defined with the value, so also on php the use of this var had to be checked.
I think it would be beneficial to change all uses of $txt['lang_rtl'] == true to !empty($context['right_to_left']). Then that way the code in Load.php can handle the comparison safely.
The code in Load.php can then be made to do this:
$context['right_to_left'] = !empty($txt['lang_rtl']) && $txt['lang_rtl'] !== 'false';
That way it should handle making it RTL for 1, '1', true and 'true'. All others should make it false.
I don't think I see anything unsafe about the comparison... Plus, it tests perfectly. Not sure any further changes are necessary.
Do you have a test case that is failing?
I do not. I would rather we didn't use loose type hinting if possible, but I am not going to push it for the 2.1 release to fix. For a future SMF release we should correct this.
Loose types are used throughout SMF. Anywhere we use == instead of ===. Also in many places we use empty(). (Though I'm aware some academic type folks want to do away with empty() for that very reason...) It's a core feature of PHP.
If I understand the issue correctly, the problem is that Crowdin is changing $txt['lang_rtl'] = true; to $txt['lang_rtl'] = 'true'; when we export the language packs.
If so, I suggest that we do something like @jdarwood007's idea, but even simpler. Early in loadLanguage() do this:
$txt['lang_rtl'] = $txt['lang_rtl'] == 'false' ? false : !empty($txt['lang_rtl']);
I believe this is the cleanest and least invasive solution possible. On the one hand, we won't need to make any other changes to SMF's codebase. On the other hand, we also won't need to change the existing values in the language files, nor will we need to futz around with Crowdin's behaviour.
The problem is that CrowdIn won't let them translate the boolean at all. So they cannot specify a language as rtl in Crowdin, not even as 'false' or 'true'. The field in CI cannot be updated.
ALL languages are stuck in ltr...
The vendor suggested using '0' and '1', which works. More discussion & info is on the internal thread (topic 583277).
I added screenshots of before/after behavior, both in CrowdIn and in SMF, above. Hope this helps.
Back to @jdarwood007 's initial note above, I did not test installs & upgrades in an rtl language, however.
Based on @jdarwood007's list above, I think the following additional changes are necessary to cover everything we need:
- Change
falseto"0"here as well: https://github.com/SimpleMachines/SMF/blob/aee494fe17322c24c2076cc084b3f4559e61db5b/Themes/default/languages/Install.english.php#L6 - Change
'true'and'false'to"1"and"0"on this line: https://github.com/SimpleMachines/SMF/blob/aee494fe17322c24c2076cc084b3f4559e61db5b/Sources/ManageLanguages.php#L1007 - (Optional) In Load.php's
loadLanguage()and in install.php's and upgrade.php'sload_lang_file(), explicitly cast$txt['lang_rtl']to boolean. We don't need to do this for our own code, but just in case some mod uses strict comparison on it somewhere, it wouldn't hurt.
FYI, while PHP is loosely typed system. They have been lately tightening that up especially with internal functions. While I don't see them breaking this, we should work towards being more strict about our type handling and consistent. PHP 8.0 min requirements in the future will greatly help with this.
OK, requested changes made. Installer, upgrader & app tested in arabic (after manually switching it to rtl, since the changes aren't in CrowdIn yet...). Tested under php 8.1.2, mysql 8.0.20.
Note that for the change in ManageLanguages.php, I changed the datatype of the rtl string to string, rather than redefine what 'bool' meant... I felt that was more logical.
Install:
Upgrade:
I don't think that approach to ManageLanguages.php will work. The UI presents a checkbox, so treating the input as a string won't work. We need to write either '0' or '1' depending on whether the checkbox was ticked or not.
Yep...
Why on earth would someone edit the strings there... Never saw those forms before. I'd almost prefer to kill those forms, tbh.
Well for our released product, we have it. Maybe for the future, we drop it in favor of supporting Crowdins in place editing UI? This of course doesn't work well for customizations. I wonder if we could somehow allow them to be supported.
It turns out that the form adapts to the field type. Since it was changed to string in the code, it prompts for a string now, not a checkbox.
I do, however, need to update the help text, which uses the term 'checkbox'...
