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

Jed format appears incorrect #90

Closed
g-johnson opened this issue Jan 20, 2016 · 8 comments
Closed

Jed format appears incorrect #90

g-johnson opened this issue Jan 20, 2016 · 8 comments

Comments

@g-johnson
Copy link

The method “Gettext\Generators\Jed” does not appear to create a valid Jed Json structure. For non-plural translations it returns an extra empty string in the translation array:

{
  "messages": {
    "orig singular 1": [
      "",
      "trans singular 1"
    ],
    "orig singular 2": [
      "orig plural 2",
      "trans singular 2",
      "trans plural 2"
    ]
  }
}

To work properly with Jed apparently requires the following format:

{
  "messages": {
    "orig singular 1": [
      "trans singular 1"
    ],
    "orig singular 2": [
      "trans singular 2",
      "trans plural 2"
    ]
  }
}

This patch to src/Generators/PhpArray:toArray method seems to resolve the issue:

/*
        foreach ($translations as $translation) {
            $key = ($translation->hasContext() ? $translation->getContext().$context_glue : '').$translation->getOriginal();
            $entry = array($translation->getPlural(), $translation->getTranslation());

            if ($translation->hasPluralTranslation()) {
                $entry = array_merge($entry, $translation->getPluralTranslation());
            }

            $array[$key] = $entry;
        }
*/
        foreach ($translations as $translation) {
            $key = ($translation->hasContext() ? $translation->getContext().$context_glue : '').$translation->getOriginal();

            if ($translation->hasPluralTranslation()) {
                $array[$key] = array_merge(array($translation->getTranslation()), $translation->getPluralTranslation());
            } else {
                $array[$key] = array($translation->getTranslation());
            }
        }

Doing something wrong or is a patch needed?

@oscarotero
Copy link
Member

mmm, interesting...
Plural original string is stored in the key "0", but it's never used, so, it could be removed. The problem is backward compatibility, becasue PhpArray is used not only for Jed, but also for the generators phpArray and jsonDictionary.
I'm not feeling bad about making this change, but this affects to these generators and even the Translator class (that used the same structure https://github.com/oscarotero/Gettext/blob/master/src/Translator.php#L192)

@g-johnson
Copy link
Author

I have it working with the patch, but other people who would like to use the utility with Jed will certainly desire this fixed.

After I posted the initial issue, I noticed that the plural structure was not working as well. I edited the original post to reflect this.

I understand your desire not to modify the phpArray structure. Maybe the array could be refactored in the Jed method just before it is Json’ed.

@oscarotero
Copy link
Member

Maybe a good solution is to provide a way to convert the old array structure to the new one. So, people upgrading this library can easily update their phpArray files. Maybe an oldPhpArray extractor or something like that.

@oscarotero
Copy link
Member

Hello, @g-johnson
After thinking a little more, I decided to change only Jed generator/extractor instead all phpArray dependencies. Not only because backward compatibility but also to maintain compatibility between phpArray and other formats (For example, po format has the original plural string too, so it's better to convert array to po and po to array).

Before release this patch, can you test if Jed format works fine now, please?
Feel free also to make a Pull request if you see something to change.
Thanks

@g-johnson
Copy link
Author

The \gettext\Generators\Jed: toString is inheriting the \gettext\Generators\PhpArray:toArray method which in turn uses the incorrect PhpArray:buildArray method. Cloning the buildArray method from PhpArray to Jed fixes this inheritance problem although there may me a more elegant way.

Thank you for your prompt response to this issue...

oscarotero added a commit that referenced this issue Jan 20, 2016
@oscarotero
Copy link
Member

Ok, I made a mistake putting self instead static.

The only differente between Jed and PhpArray is the block you've put in this issue, so I've created a method with this code https://github.com/oscarotero/Gettext/blob/master/src/Generators/PhpArray.php#L59 and Jed generator extends phpArray overriding this method https://github.com/oscarotero/Gettext/blob/master/src/Generators/Jed.php#L22
This avoid code duplication.

@g-johnson
Copy link
Author

The Jed Generator is working correctly now and Jed is translating both singular and plural text perfectly.

Your prompt assistance was much appreciated!

The issue can be closed.

@oscarotero
Copy link
Member

Good to know!
thank you for the feedback.

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

No branches or pull requests

2 participants