8000 [Translation] added <tool> element metadata to XliffFileDumper by timwhitlock · Pull Request #15225 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Translation] added <tool> element metadata to XliffFileDumper #15225

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[Translation] added <tool> element metadata to XliffFileDumper #15225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/Symfony/Component/Translation/Dumper/XliffFileDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ class XliffFileDumper extends FileDumper
*/
private $defaultLocale;

/**
* Generator metadata for <tool> element.
*
* @var array
*/
private $toolMeta = array(
'tool-id' => 'symfony',
'tool-name' => 'Symfony',
);

/**
* {@inheritdoc}
*/
Expand All @@ -39,6 +49,21 @@ public function dump(MessageCatalogue $messages, $options = array())
parent::dump($messages, $options);
}

/**
* Set XLIFF tool element metadata.
*
* @link http://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#tool_elem
*
* @param array attributes supported by the <tool> element
*/
protected function setToolMetadata( array $meta ){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why making it protected ? IT should be private IMO

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OskarStark The purpose is for translation software when presented with a file to know that Symfony generated it. Knowing this means the software can process it differently to how it might by default. I could elaborate on that, but in general if a file format provides a generator declaration, I see no reason not to use it. Also other projects using the Translation component may have their own requirement for declaring themselves.

@stof making the method private is fine for the Symfony case, but I was under the impression that Components were supposed to be portable. As shown in the test case, a supposed XliffExtendedFileDumper sets its own tool signature, hence why I made it protected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timwhitlock we start with marking methods private and only open them if someone with a good use case asks for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timwhitlock making things protected is generally the worse extension point ever (it hurts maintenance a lot because we need to maintain BC on them). It is better to design proper extension point where needed than allowing to hack into the internal logic by exposing it. Inheritance is most of the time not the proper extension point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with @stof, instead of introducing a new method here we can override those value via $options

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my initial comment I thought the maintainers would prefer to decide on best practice. I'm not experienced in contributing to Symfony, as evidenced by this discussion. If anyone maintaining the Translation component is interested in implementing this properly then great. Either way, feel free to close my PR. I'm afraid I don't have the time to rework it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timwhitlock no worry, we are here to work together :)

$required = array(
'tool-id' => 'symfony',
'tool-name' => get_class($this),
);
$this->toolMeta = array_merge( $required, $meta );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would remove the spaces

}

/**
* {@inheritdoc}
*/
Expand All @@ -57,6 +82,16 @@ protected function format(MessageCatalogue $messages, $domain)
$xliffFile->setAttribute('datatype', 'plaintext');
$xliffFile->setAttribute('original', 'file.ext');

// add generator metadata into <tool> element
if ($this->toolMeta) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can never be empty

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I must have added that clause before I added in the defaults.

$xliffHead = $xliffFile->appendChild($dom->createElement('header'));
$xliffTool = $xliffHead->appendChild($dom->createElement('tool'));
foreach ($this->toolMeta as $toolProp => $toolValue) {
$xliffTool->setAttribute($toolProp, $toolValue);
}
$xliffFile->setAttribute('tool-id', $xliffTool->getAttribute('tool-id') );
}

$xliffBody = $xliffFile->appendChild($dom->createElement('body'));
foreach ($messages->all($domain) as $source => $target) {
$translation = $dom->createElement('trans-unit');
Expand Down
29 changes: 29 additions & 0 deletions src/Symfony/Component/Translation/Tests/Dumper/XliffFileDumperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,33 @@ public function testDump()

unlink($tempDir.'/messages.en_US.xlf');
}

public function testExtendedDumperCanSetCustomToolInfo()
{
$dumper = new XliffExtendedFileDumper();

$this->assertContains(
'<tool tool-id="symfony" tool-name="Test" tool-version="0.0"/>',
$dumper->formatTestMessages( array() )
);
}

}

/**
* Test subclass for configuring dumper via protected functions.
*/
class XliffExtendedFileDumper extends XliffFileDumper {
public function formatTestMessages( array $messages ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you have all theses spaces there?

@javiereguiluz is this valid? fabbot didn't get angry 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any spaces I added were to emulate the style of the rest of the file, rather than my personal choice. I read in the community guidelines this was the expected form.

The fabbot sent me a patch on my first PR attempt and I duly applied it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, didn't know that...

i think this is not wrong, but i didn't read this style so often

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong according to PSR-2 (opening braces on the same line and spaces inside the parentheses), strange fabbot didn't complain.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I applied the patch wrongly then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof what do you say?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? http://fabbot.io/report/symfony/symfony/15225/3c23b2e9596faf0518b7ed3084b1a2fbd0505be8

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, maybe fabbot didn't check that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should remove those extra spaces

< 8000 /div>

$this->setToolMetadata(array(
'tool-name' => 'Test',
'tool-version' => '0.0',
));

$catalogue = new MessageCatalogue('en_US');
$catalogue->add( $messages, 'foo' );

return $this->format( $catalogue, 'foo' );
}
}
6 changes: 6 additions & 0 deletions src/Symfony/Component/Translation/Tests/Loader/XliffFileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,4 +139,10 @@ public function testLoadNotes()
$this->assertNull($catalogue->getMetadata('extra', 'domain1'));
$this->assertEquals(array('notes' => array(array('content' => 'baz'), array('priority' => 2, 'from' => 'bar', 'content' => 'qux'))), $catalogue->getMetadata('key', 'domain1'));
}

public function testHeaderAllowedButDoesNothing(){
$loader = new XliffFileLoader();
$catalogue = $loader->load(__DIR__.'/../fixtures/withheader.xlf', 'en', 'domain1');
$this->assertSame( array('foo' => 'bar'), $catalogue->all('domain1') );
}
}
5 changes: 4 additions & 1 deletion src/Symfony/Component/Translation/Tests/fixtures/resources-clean.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
<file source-language="fr-FR" target-language="en-US" datatype="plaintext" original="file.ext">
<file source-language="fr-FR" target-language="en-US" datatype="plaintext" original="file.ext" tool-id="symfony">
<header>
<tool tool-id="symfony" tool-name="Symfony"/>
</header>
<body>
<trans-unit id="acbd18db4cc2f85cedef654fccc4a4d8" resname="foo">
<source>foo</source>
Expand Down
14 changes: 14 additions & 0 deletions src/Symfony/Component/Translation/Tests/fixtures/withheader.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?xml version="1.0" encoding="utf-8"?>
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
<file source-language="en" target-language="en" datatype="plaintext" original="file.ext" tool-id="foo">
<header>
<tool tool-id="foo" tool-name="Foo Tool" tool-version="0" />
</header>
<body>
<trans-unit id="1">
<source>foo</source>
<target>bar</target>
</trans-unit>
</body>
</file>
</xliff>
0