From ab40dcc0ca118a782b95491eacd927a6f6f8c092 Mon Sep 17 00:00:00 2001 From: Jeremy Thomerson Date: Fri, 31 May 2013 19:24:12 +0000 Subject: [PATCH 1/4] FIX make augmentSQL API consistent for strict PHP This references silverstripe/silverstripe-translatable#113 For that issue, we needed to have the DataQuery as the second parameter to DataQuery's augmentSQL call. Fortunately, DataQuery was already passing this argument. However, where the function was defined in DataExtension, the argument was not present. Thus, subclasses of DataExtension could not add the parameter to their function signature if they were running in PHP strict mode because PHP will complain that the signatures don't match. --- model/DataExtension.php | 2 +- model/Hierarchy.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/model/DataExtension.php b/model/DataExtension.php index 041630200..795a6332c 100644 --- a/model/DataExtension.php +++ b/model/DataExtension.php @@ -42,7 +42,7 @@ abstract class DataExtension extends Extension { * * @param SQLQuery $query Query to augment. */ - public function augmentSQL(SQLQuery &$query) { + public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) { } /** diff --git a/model/Hierarchy.php b/model/Hierarchy.php index d4f6c667c..8309406fe 100644 --- a/model/Hierarchy.php +++ b/model/Hierarchy.php @@ -38,7 +38,7 @@ class Hierarchy extends DataExtension { */ private static $node_threshold_leaf = 250; - public function augmentSQL(SQLQuery &$query) { + public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) { } public function augmentDatabase() { From f5b6c552454edbfecd7f37bbbb42b45df87ad50f Mon Sep 17 00:00:00 2001 From: Sean Harvey Date: Sat, 1 Jun 2013 11:24:25 +1200 Subject: [PATCH 2/4] Updating FileTest to use the correct shortcode format with commas --- tests/filesystem/FileTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/filesystem/FileTest.php b/tests/filesystem/FileTest.php index db2d8c8b1..ed716eedc 100644 --- a/tests/filesystem/FileTest.php +++ b/tests/filesystem/FileTest.php @@ -15,8 +15,8 @@ class FileTest extends SapphireTest { $parser = new ShortcodeParser(); $parser->register('file_link', array('File', 'link_shortcode_handler')); - $fileShortcode = sprintf('[file_link id=%d]', $testFile->ID); - $fileEnclosed = sprintf('[file_link id=%d]Example Content[/file_link]', $testFile->ID); + $fileShortcode = sprintf('[file_link,id=%d]', $testFile->ID); + $fileEnclosed = sprintf('[file_link,id=%d]Example Content[/file_link]', $testFile->ID); $fileShortcodeExpected = $testFile->Link(); $fileEnclosedExpected = sprintf( @@ -27,11 +27,11 @@ class FileTest extends SapphireTest { $testFile->delete(); - $fileShortcode = '[file_link id="-1"]'; - $fileEnclosed = '[file_link id="-1"]Example Content[/file_link]'; + $fileShortcode = '[file_link,id="-1"]'; + $fileEnclosed = '[file_link,id="-1"]Example Content[/file_link]'; $this->assertEquals('', $parser->parse('[file_link]'), 'Test that invalid ID attributes are not parsed.'); - $this->assertEquals('', $parser->parse('[file_link id="text"]')); + $this->assertEquals('', $parser->parse('[file_link,id="text"]')); $this->assertEquals('', $parser->parse('[file_link]Example Content[/file_link]')); if(class_exists('ErrorPage')) { From 1cebfc5d51a6d6d06ab222f8d0354dfe02ef7ed8 Mon Sep 17 00:00:00 2001 From: Sean Harvey Date: Sat, 1 Jun 2013 11:50:23 +1200 Subject: [PATCH 3/4] Revert "FIX make augmentSQL API consistent for strict PHP" This reverts commit ab40dcc0ca118a782b95491eacd927a6f6f8c092. --- model/DataExtension.php | 2 +- model/Hierarchy.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/model/DataExtension.php b/model/DataExtension.php index 795a6332c..041630200 100644 --- a/model/DataExtension.php +++ b/model/DataExtension.php @@ -42,7 +42,7 @@ abstract class DataExtension extends Extension { * * @param SQLQuery $query Query to augment. */ - public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) { + public function augmentSQL(SQLQuery &$query) { } /** diff --git a/model/Hierarchy.php b/model/Hierarchy.php index 8309406fe..d4f6c667c 100644 --- a/model/Hierarchy.php +++ b/model/Hierarchy.php @@ -38,7 +38,7 @@ class Hierarchy extends DataExtension { */ private static $node_threshold_leaf = 250; - public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) { + public function augmentSQL(SQLQuery &$query) { } public function augmentDatabase() { From a3c406e4d2e21f10eb25084ab143dbec0b48c165 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Sun, 2 Jun 2013 20:17:28 +0200 Subject: [PATCH 4/4] NEW Merge i18nTextCollector with existing (fixes #1838) This is a necessity for any further 3.1 pushes of master files to getlocalization. Because we'd otherwise remove existing master strings for CTF etc, which means we can no longer backport new translations to 3.0 (and there's no way for users to contribute translations to 3.0 via getlocalization). It's still a very monolithic class, but at least I've refactored it to return all collected strings without writing it to files (for easier testing). --- i18n/i18nTextCollector.php | 52 ++++++++++++++++--- tasks/i18nTextCollectorTask.php | 8 ++- .../templates/i18nTestModule.ss | 3 +- tests/i18n/i18nTextCollectorTest.php | 24 +++++++++ 4 files changed, 77 insertions(+), 10 deletions(-) diff --git a/i18n/i18nTextCollector.php b/i18n/i18nTextCollector.php index da112c709..4129ba5c1 100644 --- a/i18n/i18nTextCollector.php +++ b/i18n/i18nTextCollector.php @@ -71,10 +71,21 @@ class i18nTextCollector extends Object { * @uses DataObject->collectI18nStatics() * * @param array $restrictToModules + * @param array $mergeWithExisting Merge new master strings with existing ones + * already defined in language files, rather than replacing them. This can be useful + * for long-term maintenance of translations across releases, because it allows + * "translation backports" to older releases without removing strings these older releases + * still rely on. */ - public function run($restrictToModules = null) { - //Debug::message("Collecting text...", false); - + public function run($restrictToModules = null, $mergeWithExisting = false) { + $entitiesByModule = $this->collect($restrictToModules, $mergeWithExisting); + // Write each module language file + if($entitiesByModule) foreach($entitiesByModule as $module => $entities) { + $this->getWriter()->write($entities, $this->defaultLocale, $this->baseSavePath . '/' . $module); + } + } + + public function collect($restrictToModules = null, $mergeWithExisting = false) { $modules = scandir($this->basePath); $themeFolders = array(); @@ -137,7 +148,31 @@ class i18nTextCollector extends Object { $entitiesByModule[$othermodule][$fullName] = $spec; unset($entitiesByModule[$module][$fullName]); } - } + } + + // Optionally merge with existing master strings + // TODO Support all defined source formats through i18n::get_translators(). + // Currently not possible because adapter instances can't be fully reset through the Zend API, + // meaning master strings accumulate across modules + if($mergeWithExisting) { + $adapter = Injector::inst()->get('i18nRailsYamlAdapter'); + $masterFile = "{$this->basePath}/{$module}/lang/" + . $adapter->getFilenameForLocale($this->defaultLocale); + if(!file_exists($masterFile)) continue; + + $adapter->addTranslation(array( + 'content' => $masterFile, + 'locale' => $this->defaultLocale + )); + $entitiesByModule[$module] = array_merge( + array_map( + // Transform each master string from scalar value to array of strings + function($v) {return array($v);}, + $adapter->getMessages($this->defaultLocale) + ), + $entitiesByModule[$module] + ); + } } // Restrict modules we update to just the specified ones (if any passed) @@ -147,10 +182,11 @@ class i18nTextCollector extends Object { } } - // Write each module language file - if($entitiesByModule) foreach($entitiesByModule as $module => $entities) { - $this->getWriter()->write($entities, $this->defaultLocale, $this->baseSavePath . '/' . $module); - } + return $entitiesByModule; + } + + public function write($module, $entities) { + $this->getWriter()->write($entities, $this->defaultLocale, $this->baseSavePath . '/' . $module); } /** diff --git a/tasks/i18nTextCollectorTask.php b/tasks/i18nTextCollectorTask.php index 1f39f3d5a..4e36e9038 100644 --- a/tasks/i18nTextCollectorTask.php +++ b/tasks/i18nTextCollectorTask.php @@ -10,6 +10,12 @@ class i18nTextCollectorTask extends BuildTask { protected $description = " Traverses through files in order to collect the 'entity master tables' stored in each module. + + Parameters: + - locale: Sets default locale + - writer: Custom writer class (defaults to i18nTextCollector_Writer_RailsYaml) + - module: One or more modules to limit collection (comma-separated) + - merge: Merge new strings with existing ones already defined in language files (default: FALSE) "; public function init() { @@ -32,6 +38,6 @@ class i18nTextCollectorTask extends BuildTask { $writer = $request->getVar('writer'); if($writer) $c->setWriter(new $writer()); $restrictModules = ($request->getVar('module')) ? explode(',', $request->getVar('module')) : null; - return $c->run($restrictModules); + return $c->run($restrictModules, (bool)$request->getVar('merge')); } } diff --git a/tests/i18n/_fakewebroot/i18ntestmodule/templates/i18nTestModule.ss b/tests/i18n/_fakewebroot/i18ntestmodule/templates/i18nTestModule.ss index 5cc818354..1c245b670 100644 --- a/tests/i18n/_fakewebroot/i18ntestmodule/templates/i18nTestModule.ss +++ b/tests/i18n/_fakewebroot/i18ntestmodule/templates/i18nTestModule.ss @@ -1,3 +1,4 @@ <% _t('i18nTestModule.MAINTEMPLATE',"Main Template") %> $Layout -lonely _t() call that should be ignored \ No newline at end of file +lonely _t() call that should be ignored +<% _t('i18nTestModule.NEWENTITY',"Not stored in master file yet") %> \ No newline at end of file diff --git a/tests/i18n/i18nTextCollectorTest.php b/tests/i18n/i18nTextCollectorTest.php index 888f9998b..04fcbcb35 100644 --- a/tests/i18n/i18nTextCollectorTest.php +++ b/tests/i18n/i18nTextCollectorTest.php @@ -538,6 +538,30 @@ YAML; Config::inst()->update('SSViewer', 'theme', $theme); } + + public function testCollectMergesWithExisting() { + $defaultlocal = i18n::default_locale(); + $local = i18n::get_locale(); + i18n::set_locale('en_US'); + i18n::set_default_locale('en_US'); + + $c = new i18nTextCollector(); + $c->setWriter(new i18nTextCollector_Writer_Php()); + $c->basePath = $this->alternateBasePath; + $c->baseSavePath = $this->alternateBaseSavePath; + + $entitiesByModule = $c->collect(null, true /* merge */); + $this->assertArrayHasKey( + 'i18nTestModule.ENTITY', + $entitiesByModule['i18ntestmodule'], + 'Retains existing entities' + ); + $this->assertArrayHasKey( + 'i18nTestModule.NEWENTITY', + $entitiesByModule['i18ntestmodule'], + 'Adds new entities' + ); + } public function testCollectFromFilesystemAndWriteMasterTables() { $defaultlocal = i18n::default_locale();