php-gettext / Gettext

PHP library to collect and manipulate gettext (.po, .mo, .php, .json, etc)
MIT License
687 stars 132 forks source link

4.x: Fix parsing of PO files without empty lines #296

Closed swissspidy closed 2 months ago

swissspidy commented 2 months ago

This resolves an issue that was brought to our attention in https://github.com/wp-cli/i18n-command/issues/393

Turns out that msgfmt doesn't like empty lines between translations when merging PO files, and instead adds empty comments.

So a file like this:

# File: eme-actions.php, line: 343
# File: eme-events.php, line: 9395
msgid "An error has occurred"
msgstr "Es ist ein Fehler aufgetreten"

# File: eme-actions.php, line: 344
msgid "Clear"
msgstr "Leeren"

# File: eme-actions.php, line: 345
# File: eme-events.php, line: 9396
msgid "Mailing preferences"
msgstr "Mailing Präferenzen"

# File: eme-actions.php, line: 346
# File: eme-events.php, line: 9397
msgid "Yes, I'm sure"
msgstr "Ja, ich bin mir sicher"

Gets turned into this:

#
# File: eme-actions.php, line: 343
# File: eme-events.php, line: 9395
msgid "An error has occurred"
msgstr "Es ist ein Fehler aufgetreten"
#
# File: eme-actions.php, line: 344
msgid "Clear"
msgstr "Leeren"
#
# File: eme-actions.php, line: 345
# File: eme-events.php, line: 9396
msgid "Mailing preferences"
msgstr "Mailing Präferenzen"
#
# File: eme-actions.php, line: 346
# File: eme-events.php, line: 9397
msgid "Yes, I'm sure"
msgstr "Ja, ich bin mir sicher"

As per the spec, whitespace between entries is optional. However, Gettext v4 doesn't parse these files correctly when they look like this.

This PR addresses that.

In the WP-CLI i18n command we currently rely on Gettext v4 because of PHP version requirements, so I hope this bugfix gets accepted still.

oscarotero commented 2 months ago

If I understood correctly, an empty comment is the same as an empty line, right? So a simpler approach would be:

// Line 31

if ($line === "#") {
  $line = "";
}

Does it make sense?

swissspidy commented 2 months ago

That... also works! Much simpler, thanks. Code updated ✅

oscarotero commented 2 months ago

Thanks, merged and released in v4.8.12