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

Incorrect output of _translateFormulaToEnglish() function #2533

Closed
aleks-samurai opened this issue Jan 27, 2022 · 12 comments
Closed

Incorrect output of _translateFormulaToEnglish() function #2533

aleks-samurai opened this issue Jan 27, 2022 · 12 comments

Comments

@aleks-samurai
Copy link

This is:

- [x] a bug report
- [ ] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

Incorrect output of _translateFormulaToEnglish() if function name in Russian ends with "И" and translation of this function is below line 214 in src/PhpSpreadsheet/Calculation/locale/ru/functions.

What is the expected behavior?

=ЕСЛИ(1;1;1) -> =IF(1;1;1)
=ИСКЛИЛИ(1;1) -> =XOR(1;1)

What is the current behavior?

=ЕСЛИ(1;1;1) -> =ЕСЛAND(1;1;1)
=ИСКЛИЛИ(1;1) -> =ИСКЛИЛAND(1;1)

What are the steps to reproduce?

Please provide a Minimal, Complete, and Verifiable example of code that exhibits the issue without relying on an external Excel file or a web server:

<?php

require __DIR__ . '/vendor/autoload.php';

// Create new Spreadsheet object
$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();

echo Calculation::getInstance()->_translateFormulaToEnglish("=ЕСЛИ(1;1;1)");

### Which versions of PhpSpreadsheet and PHP are affected?
// php 7.4, phpoffice/phpspreadsheet 1.21
@aleks-samurai aleks-samurai changed the title Incorrect output of _translateFormulaToEnglish() Incorrect output of _translateFormulaToEnglish() function Jan 27, 2022
@oleibman
Copy link
Collaborator

This is an interesting problem. The translations use regular expressions to find the function name preceded by a non-word character. This works for Latin alphabets, but not so well for Cyrillic. I think that, instead of \w, the regexp could use \p{L} to match a letter in any script (and add 0-9_ to pick up the rest of the word characters); I believe a u (Unicode) modifier is also needed, but the i (case insensitive) modifier would no longer be needed. I need to do more testing to validate all of this.

@aleks-samurai
Copy link
Author

@oleibman Thanks for the reply. I hope this bug will be fixed in near future. Thanks for your work.

@MarkBaker
Copy link
Member

MarkBaker commented Jan 28, 2022

I'm afraid the formula language translation code is a bit basic, dating back to when a only very few languages were available; and the code may not handle languages that have been added more recently. The regexp should probably be greedy and check for a word boundary, and should (as @oleibman says) use the /u modifier.

It might also be useful if you could check the contents of the ru language set in /src/PhpSpreadsheet/Calculation/locale/Translations.xlsx which is used to generate the locale settings for each language. I updated this last year, but it's never easy extracting the function names from MS Excel by hand to build that spreadsheet (trying to double check everything in the documentation in each language); and as I don't understand Russian, it's difficult to know if I've made any mistakes or not. It doesn't help that some languages have functions missing, or additional functions that aren't available in other locales.

@MarkBaker
Copy link
Member

It would probably also be neater and less risky if it was modified to use a preg_replace_callback() as well. I'll take a look at it tonight. I've already added tests using your examples

@MarkBaker
Copy link
Member

MarkBaker commented Jan 28, 2022

On the good news side... the fact that you've identified this issue justifies the effort in providing a mechanism to handle formula translations, and in maintaining the translations language locale data

@aleks-samurai
Copy link
Author

aleks-samurai commented Jan 28, 2022

On the good news side... the fact that you've identified this issue justifies the effort in providing a mechanism to handle formula translations, and in maintaining the translations language locale data

I will provide you updated /src/PhpSpreadsheet/Calculation/locale/Translations.xlsx file for the ru locale ASAP, because I see the functions provided in Excel and Microsoft 365 supported by your package, but not translated to ru.

@MarkBaker
Copy link
Member

I will provide you updated /src/PhpSpreadsheet/Calculation/locale/Translations.xlsx file for the ru locale ASAP, because I see the functions provided in Excel supported by your package, but not translated to ru.

Thanks, much appreciated; every new release of Excel seems to add new functions, or new aliases for existing functions

@aleks-samurai
Copy link
Author

I will provide you updated /src/PhpSpreadsheet/Calculation/locale/Translations.xlsx file for the ru locale ASAP, because I see the functions provided in Excel supported by your package, but not translated to ru.

Thanks, much appreciated; every new release of Excel seems to add new functions, or new aliases for existing functions
Translations.xlsx
Updated file. Cells with orange-yellow background - additions, cells with red background (only one) - changes. I also checked the function naming in Russian. Everything is ok, no mistakes.

@aleks-samurai
Copy link
Author

@MarkBaker @oleibman Any news?

@MarkBaker
Copy link
Member

The code change is now in the master branch; and your additional function translations will be built from the spreadsheet on the next release (thank you for that)

@aleks-samurai
Copy link
Author

Изменение кода теперь находится в masterветке; и ваши дополнительные переводы функций будут построены из электронной таблицы в следующем выпуске (спасибо за это)

Thank you guys!

@oleibman
Copy link
Collaborator

Issue seems resolved; closing ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants