When exporting po file templates, with the default option (Source text only, no translation) I've found these issues:

1. We get duplicated strings.
To get this, you just need to add two languages and add a translation of the same string for each.

Looking into the query, it seems either there's one DISTINCT missing or one join not needed (we wouln't need to join locales_target here).

2. Then, reloading the page, we get multiple warnings like these:

Warning: addcslashes() expects parameter 1 to be string, array given in Drupal\Component\Gettext\PoItem->formatString() (line 269 of core/lib/Drupal/Component/Gettext/PoItem.php).

I suspect this last one is related to the singular / plural handling in PoItem class.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-ui

Adding D8MI tags.

Jose Reyero’s picture

The 'duplicated strings' issue will be fixed by #1785086: Introduce a generic API for interface translation strings

Jose Reyero’s picture

So, duplicated strings fixed, now there's only the export warnings.

Sutharsan’s picture

Title: Duplicated strings and warnings when exporting templates: addcslashes() expects parameter 1 to be string » Warnings when exporting templates: addcslashes() expects parameter 1 to be string

Updating title to match the issue scope.

Outi’s picture

Just to notice that the problem Jose Reyero describes still occurs – in the end of the exportation of a .po file, I get this message:

Warning: addcslashes() expects parameter 1 to be string, array given in Drupal\Component\Gettext\PoItem->formatString() (line 269 of core/lib/Drupal/Component/Gettext/PoItem.php).

Gábor Hojtsy’s picture

Oh, sounds like an issue with plural exports? can you look in the exported .po file if there are msgid and msgid_plural items that are empty? Or suspicious in some way?

Outi’s picture

We checked with Gábor Hojtsy the msgid and msgid_plural items and they seemed to be normal.

I wanted to add that I had this problem in these conditions:
- I installed the site in French
- on admin/config/regional/translate I translated some strings from English to French
- on admin/config/regional/translate/export I exported these translations on a .po file
- I go the error message

The problem doesn't occur when
- I install the site in English
- I don't translate anything and on admin/config/regional/translate/export I have this message: "No language available. The export will only contain source strings."
- I click anyway on the "export" link and get the drupal.pot file
- I don't have the error message

droplet’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
2.29 KB

Also, symfony required php_fileinfo ext to output to files. Is there somewhere we can/should document it ?

droplet queued 8: localeExport.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: localeExport.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
698 bytes

I've found these warnings again when exporting translations (Spanish). This happened using a custom script though, so not sure where this fixes the core issue too.

Apparently this was happening with plural strings that had just one of them translated (instead of the two) -- We have two plural forms in Spanish, just like in English.

When setting the values with PoItem::setFromArray(), it was checking whether there were more than one translations to set it as plural or not. The patch changes this to check whether there are more than one source strings --instead of translations.

In the resulting po trings that were not properly exported before resulting in

msgid ""
msgstr ""

+ Warning

Are now exported as:

msgid "@count item"
msgid_plural "@count items"
msgstr[0] "@count elemento"

One line patch, easy to review !! :-)

Status: Needs review » Needs work

The last submitted patch, 11: 1789090-warnings_exporting_locales.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
666 bytes

Re-rolled patch with the right root folder so testbot can apply it.

Gábor Hojtsy’s picture

Issue tags: +sprint, +Needs tests

A test would be great for this one. Also the issue summary has one more issue explained. Is that still in the scope here?

Jose Reyero’s picture

About the first issue (1. Duplicated strings) it should be already fixed, see #2

About the second one (2. Warnings when exporting pot), not exactly the same than "when exporting translations", but this patch fixes, it is the same issue AFAIK. With the patch applied, warnings are gone.

So I think this one is almost RTBC, it just needs that tests -no time for them myself atm.

droplet’s picture

Status: Needs review » Needs work

Doesn't it same to my patch #8 ?

Jose Reyero’s picture

@droplet,

Right, I think it is the same and I see you've also added some improved tests. Though it seems those tests don't check for this specific issue.

So if you could update the patch for it to apply cleanly, and maybe also add a test for this one, that would be great. (I think just adding some string that doesn't have both plural forms translated would do).

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

re-roll of #8

Jose Reyero’s picture

Latest patch looks good to me, just found a typo, otherwise it's RTBC

+   * @return string
+   *   A .po file fragment with an translated string.

+ * A .po file fragment with a translated string.

vijaycs85’s picture

FileSize
653 bytes
2.25 KB

Updating...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, let's get this in then! Thanks all!

Gábor Hojtsy’s picture

Issue tags: -Needs tests

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 1789090-20.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Fail on migratetermnodetest.

Gábor Hojtsy queued 20: 1789090-20.patch for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Just applying the test and running it does not result in fail so whatever bug we're fixing here is not tested yet by the test.

Jose Reyero’s picture

About the test, I think what we need to test is what happens when you only add one of the two plural translations, then click on save, then export the po.

alexpott’s picture

rodrigoaguilera’s picture

Status: Needs work » Fixed

This has been fixed at #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list and I also can't reproduce the duplicated strings nor the warnings.

I don't know if the test makes sense if we cannot make it fail.

Gábor Hojtsy’s picture

Status: Fixed » Closed (duplicate)
Issue tags: -sprint, -Needs tests

Yeah the PoItem change is there. Closing as duplicate instead then. It is unfortunate that credit from this issue did not make it to https://www.drupal.org/commitlog/commit/2/e7a7de9f8cf29bb5d4593eccd44659... :/