inpsyde / wp-translation-downloader

Composer plugin to download WordPress translations
MIT License
45 stars 3 forks source link

Handle the case if the translation already exists #9

Closed MaximeCulea closed 2 years ago

MaximeCulea commented 3 years ago

Describe the bug When a translation has already been downloaded, on update/install packages, the "same" translation can't handle the fact it's already existing. There is like a prompt to ask user choice, but no prompt is asked, the script skips alone to the next translation.

To Reproduce Steps to reproduce the behavior:

  1. Lunch the composer install/update once
  2. Again, to lunch the same cases
  3. See error :
    
    twentytwenty: found 1 translations
    Failed to execute (1) unzip -qq  '/home/mculea/.cache/composer/translations/twentytwenty-fr_FR-1.6.zip' -d '/home/mculea/Documents/Projects/wordpress-skeleton/web/app/languages/themes/'

replace /home/mculea/Documents/Projects/wordpress-skeleton/web/app/languages/themes/twentytwenty-fr_FR.po? [y]es, [n]o, [A]ll, [N]one, [r]ename: NULL (EOF or read error, treating as "[N]one" ...)

The archive may contain identical file names with different capitalization (which fails on case insensitive filesystems)
Unzip with unzip command failed, falling back to ZipArchive class
✓ 1.6 | fr_FR


**Expected behavior**
Prompt is working by asking what to do or even better, a global arg which tell how to systematically handle this case. overwrite: true|false|ask.

**System (please complete the following information):**
 - OS: Windows
 - Version 2.0.2
soderlind commented 3 years ago

Having the same problem. I suggest that you add a parameter like:

"replace" : "[yes|no|All|None|rename]"

as in:

{
    "name": "vendor/my-package",
    "extra": {
        "wp-translation-downloader": {
            "languages": [
                "de_DE"
            ],
            "replace": "yes",
            "directory": "public/wp-content/languages"
        }
    }
}
MaximeCulea commented 3 years ago

@soderlind Smart!

It's not a bugfix, more a workarround, but at least I'll take that 🤗

soderlind commented 3 years ago

Thank you, and I agree, it's not a bug it's an enhancement :)

Chrico commented 3 years ago

Good morning ☕

Our package tries to use the system unzip-comand first to unzip the files. This comand fails, because the files already exists in the target folder. But afterwards it is falling back to PHP ZipArchive - you can see that right below the prompt message:

    Unzip with unzip command failed, falling back to ZipArchive class
✓ 1.6 | fr_FR

If ZipArchive also fails, then we're trying to run unzip with the -o argument as "last chance". Same does composer as well. The problem i see with implementing the new option or using the flag straight away is, that it seems not to be recommended to use that:

-o

overwrite existing files without prompting. This is a dangerous option, so use it with care. (It is often used with -f, however, and is the only way to overwrite directory EAs under OS/2.)

Also for ZipArchive::extractTo(), we have no possibility to use that option replace. So this would introduce some inconsistency.

soderlind commented 3 years ago

According to https://www.javaer101.com/en/article/15338356.html there's a bug in ZipArchive, you should pass the flags ZIPARCHIVE::CREATE | ZIPARCHIVE::OVERWRITE when you open the zip.

https://github.com/inpsyde/wp-translation-downloader/blob/e37ea671dd33689eb49e90b57290e45599de5e7e/src/Util/Unzipper.php#L188

Chrico commented 3 years ago

Oh good to know, but seems to be related to PHP 5.x.

As far as i could see for now:

Composer v2: https://github.com/composer/composer/blob/2.0/src/Composer/Downloader/ZipDownloader.php#L181

Composer v1 has similar. Hm..i don't think this is critical, because it only occurs when you create a new archive. But we're actually reading from an existing archive the files.

gmazzap commented 3 years ago

Having a similar issue.

First of all, Composer has a IOInterface::isInteractive() method. When that is true, Composer is being executed on a non-interactive shell, and so letting commands ask for input is pointless, and will likely end up in a failure.

In great majority of cases, in my experience, if a translation exists for a given package version, there's no need to replace if the package version was not be updated.

In fact, think that could be the default behavior with the possibility to be change the behavior via configuration.

I would suggest to have a overwrite-existing configuration which could work like this:

overwrite-existing value behavior description
"auto" (default) Translations are overwitten for packages updated since last command execution, not overwitten for other packages
"ask" A question is asked in terminal to owerwrite existing translations. When in not-interactive mode, would fallback to "auto"
true Translations are always overwitten for all packages 1
false Translations are never overwitten for any package
["some-name", "some-prefix-*"] Translations are always overwitten for specified packages. For other packages, "auto" behavior applies

To make this happen, translation downloader should keep track of which version of translation is installed, and that could be done in a wp-translation-dowloader.lock file.

That would make the whole process much faster when overwrite-existing would be set to false or to "auto" (that is the default).


1 When this option is used, I think would be safe to use unzip with the -o flag. The reason that flag is discouraged is that it might cause the overwriting of files. But considering translation files are not edited manually, and considering this option needs to be enabled on purpose, I don't see that as an issue. And in any case, using ZipArchive to overwrite the files could cause the exact same data loss...

Chrico commented 3 years ago

To make this happen, translation downloader should keep track of which version of translation is installed, and that could be done in a wp-translation-dowloader.lock file.

Just a bit context regarding the API's and the response:

The API response has a "version"-string. But the zip files for translations are always generated via WP Cron when a translation is updated. On wordpress.org-API you need additionally at least 90% of the strings translated (e.G. wordpress-core "ary" is still in version 4.8.x). You cannot do "translation releases" and you can also build those ZIP files via WP CLI, --> Overall this means, that the "version"-string points to the code version (called "project") but not to the translation version.

Comparing files wouldn't work here because every language can have multiple files.

what number of files link
wp-core 57 (ary) https://api.wordpress.org/translations/core/1.0/?version=100
wordpress-seo 22 (af) https://api.wordpress.org/translations/plugins/1.0/?slug=wordpress-seo&version=100

Long story short: ( 😬 ) What we might could do is using the "updated"-field in API response for the wp-translation-downloader.lock.


@gmazzap if you want, we can go forward with a wp-translation-downloader.lock file which contains the package, language and "updated"-field from API response. On next run we could compare with API response and only update files which are older then the updated-field. Something like:

{
    "{package-name}": {
        "translations": {
                "{iso}": {
                    "updated": "{updated}"
                }
        } 
   }
}

The idea is to not have a mapping of {package-name}.{iso} to {updated} directly for the case, we might want to enhance the lock-format lateron. By adding in the structure translations as node and also updated as object-value of {iso} we keep it more flexible for future changes.

gmazzap commented 3 years ago

Thanks @Chrico

I understand that translation version is not code version, but when you pull a 3rd party plugin from version (e.g. wp.org plugin) very rarely you need to update the translation if the code version is the same.

For custom GlotPress installations, that might be different, and if we have a better way to indetify changes, why not.

Chrico commented 2 years ago

I've implemented the Locker feature and it is part of the latest 2.1 version. Check out the changelog: https://github.com/inpsyde/wp-translation-downloader/releases/tag/2.1 :)

I'll keep that issue a bit open if anything related to that appears again. 👍