[WIP]: Allow Drupal to use Translate component #4249

clemens-tolboom · 2012-05-10T13:55:05Z

This WIP will be followed by smaller PRs as that will speedup the commit progress.
(PRs done: #4336 #4337 #4339)

This patch (still) provide for:

A Gettext class which allow for header and context support. Each PO file may have a header and can contain different msgctxt or domains. To load all domains we need to know the domains available.

PoFileLoader and PoFileDumper can use a Gettext header. The header is loaded into

$messages['messages']['__HEADER__]

PoFileLoader loads the Gettext msgctxt values into

$messages['messages']['__CONTEXT__']

To load all domains the default domain must be loaded first then each context can loaded. This can be accomplished by:

  $loader = new PoFileLoader();
  $catalogue = $loader->load($resource, $langcode);
  $all = array();
  $messages = $catalogue->all('messages');
  $domains = Gettext::getContext($messages);
  $all['messages'] = $messages;
  foreach($domains as $domain) {
    $catalogue = $loader->load($resource, $langcode, $domain);
    $messages = $catalogue->all($domain);
    $all[$domain] = $messages;
  }
  $messages = $all;

MessageSelector is refactored to give PoFileDumper access to it's rules selection. This way PoFileDumper can decide how to dump a given message.

PoFileDumper merges plural messages back into one.

As Gettext specification does not support Interval rules so these are dumped in Gettext singular form by PoFileDumper.

Open issues:

The meta data introduced by Gettext is not generic enough. Both __HEADER__ and __CONTEXT__ are injected into the message array.
Gettext comments are still not supported as that is kinda meta data too.
We do have MoFileLoader and MoFileDumper too.

travisbot · 2012-05-10T13:58:13Z

This pull request passes (merged f619d3a2 into fae4523).

stloyd · 2012-05-10T13:58:26Z

src/Symfony/Component/Translation/Loader/PoFileLoader.php

@@ -18,6 +18,8 @@
 */
 class PoFileLoader extends ArrayLoader implements LoaderInterface
 {
+    private $_header;


_ should be removed.

stloyd · 2012-05-10T14:00:27Z

You need to follow Symfony2 CS, also you should squash your commits (all those merge's should not exist in PR).

travisbot · 2012-05-10T14:07:35Z

This pull request passes (merged 57d8e125 into fae4523).

clemens-tolboom · 2012-05-10T14:14:06Z

@stloyd : tnx

I did no stash as I'm not sure whether all code is ok. Cherrypicking is now possible. And never did git stash.

Do you have a good workflow guide for stashing?

goba · 2012-05-10T14:30:27Z

Note that this does not have the legacy of the Drupal history (GPL) that the previous one at #4235 had.

stof · 2012-05-10T14:32:02Z

src/Symfony/Component/Translation/Loader/PoFileLoader.php

@@ -38,9 +40,47 @@ public function load($resource, $locale, $domain = 'messages')
        return $catalogue;
    }

+    public function setHeader($header) {


the curly brace should be on its own line for methods and classes. See http://symfony.com/doc/current/contributing/code/standards.html

stloyd · 2012-05-10T14:33:56Z

@clemens-tolboom I think this: http://www.silverwareconsulting.com/index.cfm/2010/6/6/Using-Git-Rebase-to-Squash-Commits is quite good one.

Also you can read what we have in Symfony2 docs: http://symfony.com/doc/current/contributing/code/patches.html#rework-your-patch

travisbot · 2012-05-11T15:07:53Z

This pull request passes (merged 2e668e7b into 554e073).

clemens-tolboom · 2012-05-11T15:10:00Z

I followed along the squash guide @stloyd advised. Tnx for the pointers.

I added a new class Gettext as both Po Loader/Dumper and probably Mo Loader/Dumper can use that too.

The Gettext Header is now placed into the messages under __HEADER__ and has the full header string as it's translation.

It can be deleted from the message when using a catalog to load. I'm not sure how to clean it from the Translation messages. So feedback is welcome on that.

travisbot · 2012-05-13T11:39:21Z

This pull request passes (merged 215961bf into e193452).

clemens-tolboom · 2012-05-13T11:50:27Z

I added Header support for both PoFileLoader and PoFileDumper by introducing a Gettext class.

Two tests are marked as Skipped as both indexed as interval messages have issues related to the loaded versus the dumped plural form messages.

I fixed PoFileLoader which did a interval instead of a indexed message plural handling.

But when loading and next dumping PO files:

the singular source string gets detached from it's plural form. My guess is translators don't like that.
dumping interval translation strings is not understand by PO editor. A solution could be to dump these just as a singular form.

So I'm not sure Drupal can use the current version. @goba can tell more about that.

Who can test the current version with some real datasets?

I continue working on Drupal : Let Symfony Translation component handle language messages

travisbot · 2012-05-13T12:53:14Z

This pull request passes (merged 82ef4e55 into e193452).

ghost · 2012-05-13T14:40:24Z

the singular source string gets detached from it's plural form. My guess is translators don't like that.
dumping interval translation strings is not understand by PO editor. A solution could be to dump these just as a singular form.

@clemens-tolboom What do you mean by this?

travisbot · 2012-05-13T15:27:55Z

This pull request passes (merged b521568a into e193452).

clemens-tolboom · 2012-05-13T15:31:54Z

src/Symfony/Component/Translation/Tests/fixtures/full.po

+"Content-Transfer-Encoding: \n"
+"Plural-Forms: \n"
+
+msgid "One sheep"


@Drak

Loading this translation gives two entries into our internal $messages.

array( 'One sheep' => 'un mouton', '@count sheeps' => 'un mouton|@count moutons' );

But the PoFileDumper should merge these back into one Gettext item. I fixed this in my latest push partly as the solution taken is searching for 'un mouton' then replace that while dumping into a single unit with its plural form. See PoFileDumper::extractSingulars.

But what if $messages contains two (2) translation entries with 'un mouton'? We cannot merge those can we?

I hope I documented that code good enough.

clemens-tolboom · 2012-05-13T15:47:20Z

The dumping of an Interval is done into a singular form. I'm not sure whether that is parsed back correctly. That is first dump then load it again. So that needs a test.

Is it ok to always skip PoFileDumperTest::testDumpFullInterval?

clemens-tolboom · 2012-05-14T07:26:11Z

I've closed the other (GPL troubled version) #4235

clemens-tolboom · 2012-05-14T09:52:25Z

src/Symfony/Component/Translation/MessageSelector.php

-            } else {
-                $standardRules[] = $part;
-            }
-        }


These lines are moved to a static method MessageSelector::getRules() for PoFileDumper to use.

travisbot · 2012-05-15T14:56:18Z

This pull request passes (merged 21af4183 into 563f77a).

travisbot · 2012-05-16T13:05:50Z

This pull request passes (merged 5df5fad0 into 5314836).

travisbot · 2012-05-16T14:30:15Z

This pull request passes (merged 4b7e853e into 5314836).

clemens-tolboom · 2012-05-16T14:44:45Z

Sorry for the noise ... not sure how to handle that FileDumper fix. (I need it for Drupal)

Any pointers in how to handle several branches and squash between them?

travisbot · 2012-05-16T14:46:59Z

This pull request passes (merged 518c38d9 into 5314836).

travisbot · 2012-05-18T11:41:54Z

This pull request passes (merged 5134c63c into 1e15f21).

clemens-tolboom · 2012-05-19T10:55:18Z

See also pull request Fixed PoFileLoader pluralhandling from interval to index.

clemens-tolboom · 2012-05-19T11:12:43Z

See also pull request Fixed for allowing empty translation.

travisbot · 2012-05-19T12:10:08Z

This pull request passes (merged 223b8341 into 58b9245).

clemens-tolboom · 2012-05-19T16:12:11Z

@stof thanks for the code review.

@fabpot thanks for the 3 PR merges.

I'm not satisfied from a Symfony point of view with the current PR solution concerning Metadata as __HEADER__ and __CONTEXT__ are.

It probably works for Drupal but we now have two 'metadata' items __HEADER__ and __CONTEXT__ ... are there more to come?

Drupal uses Gettext both for application translation bundles as well as a Gettext editor through ie http://localize.drupal.org/translate/languages/fr

I think Drupal can make use of ie the PhpFileDumper or other file types easy to git/diff for the staging workflow but we don't want to loose ie attribution of the translation team members which is part of a Gettext header.

I think we should extends MessageCatalogue with a metadata interface allowing for at least Gettext header data items like translation team but ie Gettext comments relating message id's to source code lines could be added as __META__ too.

I'm interested to code for that that but the related Drupal issue http://drupal.org/node/1570346 should take all my spare time first :)

It would be great to have the current PR committed for Drupal ;)

clemens-tolboom · 2012-05-20T11:07:48Z

XRef [2.1][Translator] Symfony translation process & gettext

ghost · 2012-05-20T11:56:28Z

I'm not satisfied from a Symfony point of view with the current PR solution concerning Metadata as HEADER and CONTEXT are.

@clemens-tolboom - yes, I am also not very sure about this - I am not sure it is a good long term solution - the PR might suit Drupal's needs but that is addressing the issue the wrong way round IMO. Symfony needs to do this the right way and applications should have to adapt themselves to fit.

clemens-tolboom · 2012-05-20T12:29:19Z

@Drak I did not meant Drupal. This works for Gettext oriented (Po|Mo)File* stuff.

So I guess we both agree Symfony needs a way handle to Message related MetaData?

Can you give us more examples?

travisbot · 2012-05-20T15:32:38Z

This pull request passes (merged e6b769f8 into f433f6b).

clemens-tolboom · 2012-05-20T15:32:54Z

I reworked PoFileDumper as it did not do a decent job. The plural merge was based on translation instead of the source.

I fixed that by adding an extra source ID consisting of both the Gettext msgid and msg_plural. This way the reconstruction works way better.

TODO: revert the dependency on MessageSelector.

stloyd · 2012-05-20T15:58:56Z

src/Symfony/Component/Translation/Gettext.php

+     * @return array
+     *   Ordered list of Gettext keys
+     */
+    static public function headerKeys() {


IMO this should be just public static class variable.

PHP Fatal error: Arrays are not allowed in class constants

Not as constant.

variable != constant =) static public $headerKeys = array();

Do you have an example of that?

travisbot · 2012-05-20T16:33:36Z

This pull request passes (merged 216072d5 into 1407f11).

clemens-tolboom · 2012-05-20T17:29:58Z

@stloyd @stof tnx

I did php php-cs-fixer.phar fix src/Symfony/Component/Translation/

@stof

Added empty translation to support translate process. Improved Header management. Fixed CS issues reported by @stof and added some documention. Reworked Plural merge for PoFileDumper. Applied stloyd's CS feedback. Applied CS with php php-cs-fixer.phar fix src/Symfony/Component/Translation/

travisbot · 2012-05-20T17:36:46Z

This pull request passes (merged 1f26f7c into 1407f11).

ghost · 2012-05-21T11:41:54Z

@clemens-tolboom - is there any chance you can email me? email is in my profile.

clemens-tolboom · 2012-07-01T09:43:36Z

I close this PR as:

@Drak and I skyped for 1.5 hours about Gettext and Symfony.
no response for long :/
@Drak seems to cherry pick from this thus new PRs
Drupal did it another way
Have no time until September

clemens-tolboom · 2012-11-30T09:20:11Z

@Drak have you done some cherry picking in the meantime?

This PR was merged into the master branch. Commits ------- 320fb6c [Translation] changed the MetadataAwareInterface method signatures 11ff433 [Translation] fixed CS in unit tests c40db35 [Translation] removed some code that is not done anywhere else in Symfony 63719a0 [Translation] created a new interface to avoid breaking BC 23e9e65 [Translation] fixed name 5607732 [Translation] added Metadata to MessageCatalogue Discussion ---------- [MessageCatalogue] Add Metadata to MessageCatalogue rework of symfony#4399 For improving the Gettext tools (PO, MO File Loader/Dumper) we need at least storage for their meta data. This patch allows for issues below to store and process ie Po Header, Po Header Pluralisation rule. Open - [[WIP]: Allow Drupal to use Translate component](symfony#4249) - [[2.1][Translator] Symfony translation process & gettext](symfony#4245) Closed: - [[Translation] Po/MoFileLoader parse plurization rules](symfony#3023) It has 1 TODO regarding addCatalogue: it now just override old values with new.

…#4336) Commits ------- 4c4d889 Fixed PoFileLoader pluralhandling from interval to index. Discussion ---------- [BUG] PoFileLoader pluralhandling uses interval instead of index. PoFileLoaders parsed Gettext messages as interval but should have used indexed. I added index only message strings to MessageSelectorTest to reflect this. (this is part of symfony#4249) --------------------------------------------------------------------------- by travisbot at 2012-05-19T10:11:09Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1373653) (merged 4c4d889 into 58b9245).

…ony#4337) Commits ------- dd60166 Fixed for allowing empty translation. Discussion ---------- Fixed for allowing empty translation. PoFileLoader should accept empty translations. PoFileLoader calls array_filter just before returning the $messages thus filtering empty translations. For Drupal we need to be able to load and then translate incomplete PO and POT files. (this is part of [[WIP]: Allow Drupal to use Translate component](symfony#4249)) --------------------------------------------------------------------------- by travisbot at 2012-05-19T11:14:39Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1373933) (merged 5ee0b1e6 into 58b9245). --------------------------------------------------------------------------- by travisbot at 2012-05-19T12:09:48Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1374129) (merged dd60166 into 58b9245).

…ony#4339) Commits ------- fb6cf3e Allow for missing whitelines. Discussion ---------- Allow for missing whitelines. The Gettext specification allows for 'whitespace is optional' between message string. For this to work PoFileLoader needs to save found messages on more places while processing. Thus a new method is introduced. For the tests to work PoFileDumper was changed slightly to only emit white-lines when necessary. I added more documentation from the GNU gettext documentation to make the code more understandable. When [[BUG] PoFileLoader pluralhandling uses interval instead of index.](symfony#4336) this patch needs some small rework. (this is part of [[WIP]: Allow Drupal to use Translate component)](symfony#4249) --------------------------------------------------------------------------- by travisbot at 2012-05-19T12:44:19Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1374295) (merged fb6cf3e into 58b9245). --------------------------------------------------------------------------- by stof at 2012-05-19T13:19:29Z you need to rebase your branch. github tells us it cannot be merged automatically

Commits ------- 3462afc Tests for PluralizationRules. Discussion ---------- [WIP][Translations][tests] Tests for PluralizationRules. Currently we have no tests PluralizationRules. This patch is an initial one to show we have not enough langcodes in PluralizationRules. I hope this gets in so others can fix the missing langcodes. I'm waiting for [RFC [MessageCatalogue*] Add Metadata to MessageCatalogue](symfony#4399) to get in to continue working on the [[WIP]: Allow Drupal to use Translate component](symfony#4249). --------------------------------------------------------------------------- by travisbot at 2012-05-25T14:38:37Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1433558) (merged 3462afc into 023dbf8). --------------------------------------------------------------------------- by drak at 2012-07-01T09:47:05Z Is there anything pending in this PR?

Commits ------- dd60166 Fixed for allowing empty translation. Discussion ---------- Fixed for allowing empty translation. PoFileLoader should accept empty translations. PoFileLoader calls array_filter just before returning the $messages thus filtering empty translations. For Drupal we need to be able to load and then translate incomplete PO and POT files. (this is part of [[WIP]: Allow Drupal to use Translate component](symfony/symfony#4249)) --------------------------------------------------------------------------- by travisbot at 2012-05-19T11:14:39Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1373933) (merged 5ee0b1e6 into 58b92453). --------------------------------------------------------------------------- by travisbot at 2012-05-19T12:09:48Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1374129) (merged dd601662 into 58b92453).

Commits ------- fb6cf3e Allow for missing whitelines. Discussion ---------- Allow for missing whitelines. The Gettext specification allows for 'whitespace is optional' between message string. For this to work PoFileLoader needs to save found messages on more places while processing. Thus a new method is introduced. For the tests to work PoFileDumper was changed slightly to only emit white-lines when necessary. I added more documentation from the GNU gettext documentation to make the code more understandable. When [[BUG] PoFileLoader pluralhandling uses interval instead of index.](symfony/symfony#4336) this patch needs some small rework. (this is part of [[WIP]: Allow Drupal to use Translate component)](symfony/symfony#4249) --------------------------------------------------------------------------- by travisbot at 2012-05-19T12:44:19Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1374295) (merged fb6cf3ef into 58b92453). --------------------------------------------------------------------------- by stof at 2012-05-19T13:19:29Z you need to rebase your branch. github tells us it cannot be merged automatically

Commits ------- 3462afc Tests for PluralizationRules. Discussion ---------- [WIP][Translations][tests] Tests for PluralizationRules. Currently we have no tests PluralizationRules. This patch is an initial one to show we have not enough langcodes in PluralizationRules. I hope this gets in so others can fix the missing langcodes. I'm waiting for [RFC [MessageCatalogue*] Add Metadata to MessageCatalogue](symfony/symfony#4399) to get in to continue working on the [[WIP]: Allow Drupal to use Translate component](symfony/symfony#4249). --------------------------------------------------------------------------- by travisbot at 2012-05-25T14:38:37Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1433558) (merged 3462afc0 into 023dbf80). --------------------------------------------------------------------------- by drak at 2012-07-01T09:47:05Z Is there anything pending in this PR?

stloyd reviewed May 10, 2012
View reviewed changes

stof reviewed May 10, 2012
View reviewed changes

clemens-tolboom reviewed May 13, 2012
View reviewed changes

clemens-tolboom reviewed May 14, 2012
View reviewed changes

clemens-tolboom mentioned this pull request May 15, 2012

[Translation] Po/MoFileLoader parse plurization rules #3023

Merged

stloyd reviewed May 20, 2012
View reviewed changes

clemens-tolboom closed this Jul 1, 2012

fabpot mentioned this pull request Dec 23, 2013

[MessageCatalogue] Add Metadata to MessageCatalogue #6349

Merged

clemens-tolboom mentioned this pull request Dec 23, 2013

[2.1][Translator] Symfony translation process & gettext #4245

Closed

[WIP]: Allow Drupal to use Translate component #4249

[WIP]: Allow Drupal to use Translate component #4249

Conversation

clemens-tolboom commented May 10, 2012

travisbot commented May 10, 2012

Choose a reason for hiding this comment

stloyd commented May 10, 2012

travisbot commented May 10, 2012

clemens-tolboom commented May 10, 2012

goba commented May 10, 2012

Choose a reason for hiding this comment

stloyd commented May 10, 2012

travisbot commented May 11, 2012

clemens-tolboom commented May 11, 2012

travisbot commented May 13, 2012

clemens-tolboom commented May 13, 2012

travisbot commented May 13, 2012

ghost commented May 13, 2012

travisbot commented May 13, 2012

Choose a reason for hiding this comment

clemens-tolboom commented May 13, 2012

clemens-tolboom commented May 14, 2012

Choose a reason for hiding this comment

travisbot commented May 15, 2012

travisbot commented May 16, 2012

travisbot commented May 16, 2012

clemens-tolboom commented May 16, 2012

travisbot commented May 16, 2012

travisbot commented May 18, 2012

clemens-tolboom commented May 19, 2012

clemens-tolboom commented May 19, 2012

travisbot commented May 19, 2012

clemens-tolboom commented May 19, 2012

clemens-tolboom commented May 20, 2012

ghost commented May 20, 2012

clemens-tolboom commented May 20, 2012

travisbot commented May 20, 2012

clemens-tolboom commented May 20, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbot commented May 20, 2012

clemens-tolboom commented May 20, 2012

travisbot commented May 20, 2012

ghost commented May 21, 2012

clemens-tolboom commented Jul 1, 2012

clemens-tolboom commented Nov 30, 2012