[2.1] Adding simple Zend/I18n/Loader/Tmx #1992

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@lrodziewicz

A simple version of loader plus tests. I choose SimpleXML because the solution implemented into version 1 seemed for me over-complicated and libxml is enabled by default. I'm not an xpatch expert so there may be a better way of implementing it.
I not sure if the loader should report any incompatibilities with the standard eg. missing tuid attribute or seg node so I skipped it for now.

@weierophinney
Member

Waiting to merge this until we have a separate branch for 2.1 development

@Freeaqingme
Member

@weierophinney perhaps this may be a good moment to do so?

@weierophinney
Member

New feature... I'd prefer to wait to 2.1, as we're in rc phase currently.

On Saturday, July 28, 2012, Dolf Schimmel wrote:

@weierophinney perhaps this may be a good moment to do so?


Reply to this email directly or view it on GitHub:
#1992 (comment)

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@Maks3w
Member
Maks3w commented Jul 29, 2012

I added [2.1] preffix to the PR title

@Maks3w
Member
Maks3w commented Aug 6, 2012

Hi,

We have renamed the folder for tests from Zend to ZendTest.

Can you rebase your PR to catch this change?

Thanks in advance.

@DASPRiD DASPRiD commented on the diff Aug 20, 2012
library/Zend/I18n/Translator/Loader/Tmx.php
+ libxml_use_internal_errors(true);
+
+ $xml = simplexml_load_file($filename);
+
+ if(!$xml) {
+ throw new Exception\InvalidArgumentException(sprintf(
+ '%s is not a valid tmx file',
+ $filename
+ ));
+ }
+
+ $result = $xml->xpath('/tmx/body/tu/tuv[@xml:lang=\''.$locale.'\']/..');
+
+ foreach($result as $node) {
+ $attributes = $node->attributes();
+ // Silently skip the nodes that does not have the 'tuid' attribute
@DASPRiD
DASPRiD Aug 20, 2012 Member

I wouldn't skip it silently, that can result in awkward error searching.

@lrodziewicz
lrodziewicz Aug 21, 2012

At time when I wrote this I didn't see any sensible format validation in other loaders. Any suggestion how to handle it? Just an exception? I think additional data that is not translation is not a problem so we are OK to skip it. I probably should check tmx specification for that one.

@weierophinney
weierophinney Sep 14, 2012 Member

I agree with @lukaszr here -- if the only bit that you're interested in are nodes with a specific attribute, skip them. Yes, it can potentially lead to hard-to-locate errors, but if you're mixing information in the file, you have to expect that.

@DASPRiD DASPRiD commented on the diff Aug 20, 2012
library/Zend/I18n/Translator/Loader/Tmx.php
+ $xml = simplexml_load_file($filename);
+
+ if(!$xml) {
+ throw new Exception\InvalidArgumentException(sprintf(
+ '%s is not a valid tmx file',
+ $filename
+ ));
+ }
+
+ $result = $xml->xpath('/tmx/body/tu/tuv[@xml:lang=\''.$locale.'\']/..');
+
+ foreach($result as $node) {
+ $attributes = $node->attributes();
+ // Silently skip the nodes that does not have the 'tuid' attribute
+ if(isset($attributes['tuid'])) {
+ $textDomain[(string) $attributes['tuid']] = (string) $node->tuv->seg;
@DASPRiD
DASPRiD Aug 20, 2012 Member

You should test for the existence of $node->tuv->seg.

@DASPRiD DASPRiD commented on the diff Aug 20, 2012
library/Zend/I18n/Translator/Loader/Tmx.php
+
+ $xml = simplexml_load_file($filename);
+
+ if(!$xml) {
+ throw new Exception\InvalidArgumentException(sprintf(
+ '%s is not a valid tmx file',
+ $filename
+ ));
+ }
+
+ $result = $xml->xpath('/tmx/body/tu/tuv[@xml:lang=\''.$locale.'\']/..');
+
+ foreach($result as $node) {
+ $attributes = $node->attributes();
+ // Silently skip the nodes that does not have the 'tuid' attribute
+ if(isset($attributes['tuid'])) {
@DASPRiD
DASPRiD Aug 20, 2012 Member
  • Space missing after "if"
  • Why not test for isset($node['tuid']) ?
@DASPRiD DASPRiD commented on the diff Aug 20, 2012
library/Zend/I18n/Translator/Loader/Tmx.php
+ public function load($filename, $locale)
+ {
+ if (!is_file($filename) || !is_readable($filename)) {
+ throw new Exception\InvalidArgumentException(sprintf(
+ 'Could not open file %s for reading',
+ $filename
+ ));
+ }
+
+ $textDomain = new TextDomain();
+
+ libxml_use_internal_errors(true);
+
+ $xml = simplexml_load_file($filename);
+
+ if(!$xml) {
@DASPRiD
DASPRiD Aug 20, 2012 Member

Space missing after "if".

@DASPRiD DASPRiD commented on the diff Aug 20, 2012
library/Zend/I18n/Translator/Loader/Tmx.php
+ * @param string $locale
+ * @return TextDomain
+ * @throws Exception\InvalidArgumentException
+ */
+ public function load($filename, $locale)
+ {
+ if (!is_file($filename) || !is_readable($filename)) {
+ throw new Exception\InvalidArgumentException(sprintf(
+ 'Could not open file %s for reading',
+ $filename
+ ));
+ }
+
+ $textDomain = new TextDomain();
+
+ libxml_use_internal_errors(true);
@DASPRiD
DASPRiD Aug 20, 2012 Member

This won't work properly, and will likely raise notices with invalid XML files. Also you don't reset it to the original value afterwards.

@lrodziewicz
lrodziewicz Aug 21, 2012

Why so? I have tested it and it does not rise notice. Agree with the reset to the original value.

@DASPRiD DASPRiD commented on the diff Aug 20, 2012
tests/Zend/I18n/Translator/_files/translation_en.tmx
@@ -0,0 +1,30 @@
@DASPRiD
DASPRiD Aug 20, 2012 Member

Aren't we generally using dashes (-) as space equivalent in file names in ZF?

@lrodziewicz
lrodziewicz Aug 21, 2012

Well, I did based on the existing files I have found in a /_files/ directory. Should I apply this change to all of them?

@DASPRiD
DASPRiD Sep 3, 2012 Member

Can't hurt, sure.

@DASPRiD DASPRiD commented on the diff Aug 20, 2012
tests/Zend/I18n/Translator/Loader/TmxTest.php
+ public function setUp()
+ {
+ $this->originalLocale = Locale::getDefault();
+ Locale::setDefault('en_EN');
+
+ $this->testFilesDir = realpath(__DIR__ . '/../_files');
+ }
+
+ public function tearDown()
+ {
+ Locale::setDefault($this->originalLocale);
+ }
+
+ public function testLoaderFailsToLoadMissingFile()
+ {
+ $loader = new TmxLoader();
@DASPRiD
DASPRiD Aug 20, 2012 Member

I'd suggest to instantiate the loader within the setUp method and store it in $this->loader, as it never takes constructor arguments.

@lrodziewicz
lrodziewicz Aug 21, 2012

First I tried to keep it consistent with the other tests that are initialized in this way.

Isn't that more convenient to start each test with a clean instance? If we wish to do it your way, then there should be also a test that check if loading a new file set up all internals properly.

@DASPRiD
DASPRiD Sep 3, 2012 Member

The class is freshly instantiated after each test, thus it is always clean.

@lrodziewicz

@Maks3w I will try to find time for this, this evening.

@DASPRiD DASPRiD was assigned Sep 14, 2012
@weierophinney weierophinney reopened this Sep 14, 2012
@DASPRiD
Member
DASPRiD commented Sep 28, 2012

Closing as there was no activity for over a month.

@DASPRiD DASPRiD closed this Sep 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment