Skip to content

Commit

Permalink
[translation][framework-bundle] Deprecated DiffOperation
Browse files Browse the repository at this point in the history
The ``DiffOperation`` class has been deprecated and ``TargetOperation``
should be used instead, because ``DiffOperation`` has nothing to do
with 'diff', thus its class name is misleading.

Also added detailed documents for all operation interface and classes.

The following names should have consistent meanings for all operations:

The name of ``intersection`` is temporarily introduced here to explain this issue.

* [x] ``intersection`` = source ∩ target = {x: x ∈ source ∧ x ∈ target}
* [x] ``all`` = **result of the operation, depends on the operation.**
* [x] ``new`` = all ∖ source = {x: x ∈ all ∧ x ∉ source}
* [x] ``obsolete`` = source ∖ all = {x: x ∈ source ∧ x ∉ all}

The following analysis explains why ``DiffOperation`` should be deprecated.

* [x] ``all`` = source ∪ target = {x: x ∈ source ∨ x ∈ target}
* [x] ``new`` = all ∖ source = {x: x ∈ target ∧ ∉ source}
* [x] ``obsolete`` = source ∖ all = {x: x ∈ source ∧ x ∉ source ∧ x ∉ target} = ∅

This absolutely makes sense.

* [ ] ``all`` =  intersection ∪ (target ∖ intersection) = target
* [x] ``new`` = all ∖ source = {x: x ∈ target ∧ x ∉ source}
* [x] ``obsolete`` = source ∖ all = source ∖ target = {x: x ∈ source ∧ x ∉ target}

The ``all`` part is confusing because 'diff' should either mean 'relative complement' or 'symmetric difference' operation:

* ``all`` = source ∖ target = {x: x ∈ source ∧ x ∉ target}

* ``all`` = (source ∖ target) ∪ (target ∖ source) = {x: x ∈ source ∧ x ∉ target ∨ x ∈ target ∧ x ∉ source}

* ``all`` =  intersection ∪ (target ∖ intersection) = target

So the name of ``DiffOperation`` is misleading and inappropriate.
Unfortunately, there is no corresponding set operation for this class,
so it's hard to give it an apppriate name.
From my point of view, I believe the most accurate name for this class
should be ``TargetOperation`` because its result is same as the target set.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a
  • Loading branch information
zerustech committed Sep 3, 2015
1 parent d76cc03 commit 353c94d
Show file tree
Hide file tree
Showing 10 changed files with 233 additions and 111 deletions.
Expand Up @@ -12,7 +12,7 @@
namespace Symfony\Bundle\FrameworkBundle\Command;

use Symfony\Component\Console\Style\SymfonyStyle;
use Symfony\Component\Translation\Catalogue\DiffOperation;
use Symfony\Component\Translation\Catalogue\TargetOperation;
use Symfony\Component\Translation\Catalogue\MergeOperation;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
Expand Down Expand Up @@ -139,7 +139,7 @@ protected function execute(InputInterface $input, OutputInterface $output)

// process catalogues
$operation = $input->getOption('clean')
? new DiffOperation($currentCatalogue, $extractedCatalogue)
? new TargetOperation($currentCatalogue, $extractedCatalogue)
: new MergeOperation($currentCatalogue, $extractedCatalogue);

// Exit if no messages found.
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/composer.json
Expand Up @@ -29,7 +29,7 @@
"symfony/security-csrf": "~2.6|~3.0.0",
"symfony/stopwatch": "~2.3|~3.0.0",
"symfony/templating": "~2.1|~3.0.0",
"symfony/translation": "~2.7",
"symfony/translation": "~2.8",
"doctrine/annotations": "~1.0"
},
"require-dev": {
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Translation/CHANGELOG.md
Expand Up @@ -6,6 +6,11 @@ CHANGELOG

* deprecated Translator::getMessages(), use TranslatorBagInterface::getCatalogue() instead.
* added options 'as_tree', 'inline' to YamlFileDumper
* [DEPRECATION] The `DiffOperation` class has been deprecated and
will be removed in Symfony 3.0, since its operation has nothing to do with 'diff',
so the class name is misleading. The `TargetOperation` class should be used for
this use-case instead.


2.7.0
-----
Expand Down
41 changes: 33 additions & 8 deletions src/Symfony/Component/Translation/Catalogue/AbstractOperation.php
Expand Up @@ -17,38 +17,60 @@
/**
* Base catalogues binary operation class.
*
* A catalogue binary operation performs operation on
* source (the left argument) and target (the right argument) catalogues.
*
* @author Jean-François Simon <contact@jfsimon.fr>
*/
abstract class AbstractOperation implements OperationInterface
{
/**
* @var MessageCatalogueInterface
* @var MessageCatalogueInterface The source catalogue
*/
protected $source;

/**
* @var MessageCatalogueInterface
* @var MessageCatalogueInterface The target catalogue
*/
protected $target;

/**
* @var MessageCatalogue
* @var MessageCatalogue The result catalogue
*/
protected $result;

/**
* @var null|array
* @var null|array The domains affected by this operation
*/
private $domains;

/**
* @var array
* This array stores 'all', 'new' and 'obsolete' messages for all valid domains.
*
* The data structure of this array is as follows:
* ```php
* array(
* 'domain 1' => array(
* 'all' => array(...),
* 'new' => array(...),
* 'obsolete' => array(...)
* ),
* 'domain 2' => array(
* 'all' => array(...),
* 'new' => array(...),
* 'obsolete' => array(...)
* ),
* ...
* )
* ```
*
* @var array The array that stores 'all', 'new' and 'obsolete' messages
*/
protected $messages;

/**
* @param MessageCatalogueInterface $source
* @param MessageCatalogueInterface $target
* @param MessageCatalogueInterface $source The source catalogue
* @param MessageCatalogueInterface $target The target catalogue
*
* @throws \LogicException
*/
Expand Down Expand Up @@ -140,7 +162,10 @@ public function getResult()
}

/**
* @param string $domain
* Performs operation on source and target catalogues for the given domain and
* stores the results.
*
* @param string $domain The domain which the operation will be performed for
*/
abstract protected function processDomain($domain);
}
48 changes: 13 additions & 35 deletions src/Symfony/Component/Translation/Catalogue/DiffOperation.php
Expand Up @@ -11,45 +11,23 @@

namespace Symfony\Component\Translation\Catalogue;

@trigger_error('The '.__NAMESPACE__.'\DiffOperation class is deprecated since version 2.8 and will be removed in 3.0. Use the TargetOperation class in the same namespace instead.', E_USER_DEPRECATED);

/**
* Diff operation between two catalogues.
*
* The name of 'Diff' is misleading because the operation
* has nothing to do with diff:
*
* intersection = source ∩ target = {x: x ∈ source ∧ x ∈ target}
* all = intersection ∪ (target ∖ intersection) = target
* new = all ∖ source = {x: x ∈ target ∧ x ∉ source}
* obsolete = source ∖ all = source ∖ target = {x: x ∈ source ∧ x ∉ target}
*
* @author Jean-François Simon <contact@jfsimon.fr>
*
* @deprecated since version 2.8, to be removed in 3.0. Use TargetOperation instead.
*/
class DiffOperation extends AbstractOperation
class DiffOperation extends TargetOperation
{
/**
* {@inheritdoc}
*/
protected function processDomain($domain)
{
$this->messages[$domain] = array(
'all' => array(),
'new' => array(),
'obsolete' => array(),
);

foreach ($this->source->all($domain) as $id => $message) {
if ($this->target->has($id, $domain)) {
$this->messages[$domain]['all'][$id] = $message;
$this->result->add(array($id => $message), $domain);
if (null !== $keyMetadata = $this->source->getMetadata($id, $domain)) {
$this->result->setMetadata($id, $keyMetadata, $domain);
}
} else {
$this->messages[$domain]['obsolete'][$id] = $message;
}
}

foreach ($this->target->all($domain) as $id => $message) {
if (!$this->source->has($id, $domain)) {
$this->messages[$domain]['all'][$id] = $message;
$this->messages[$domain]['new'][$id] = $message;
$this->result->add(array($id => $message), $domain);
if (null !== $keyMetadata = $this->target->getMetadata($id, $domain)) {
$this->result->setMetadata($id, $keyMetadata, $domain);
}
}
}
}
}
Expand Up @@ -12,7 +12,11 @@
namespace Symfony\Component\Translation\Catalogue;

/**
* Merge operation between two catalogues.
* Merge operation between two catalogues as follows:
* all = source ∪ target = {x: x ∈ source ∨ x ∈ target}
* new = all ∖ source = {x: x ∈ target ∧ x ∉ source}
* obsolete = source ∖ all = {x: x ∈ source ∧ x ∉ source ∧ x ∉ target} = ∅
* Basically, the result contains messages from both catalogues.
*
* @author Jean-François Simon <contact@jfsimon.fr>
*/
Expand Down
22 changes: 18 additions & 4 deletions src/Symfony/Component/Translation/Catalogue/OperationInterface.php
Expand Up @@ -16,6 +16,20 @@
/**
* Represents an operation on catalogue(s).
*
* An instance of this interface performs an operation on one or more catalogues and
* stores intermediate and final results of the operation.
*
* The first catalogue in its argument(s) is called the 'source catalogue' or 'source' and
* the following results are stored:
*
* Messages: also called 'all', are valid messages for the given domain after the operation is performed.
*
* New Messages: also called 'new' (new = all ∖ source = {x: x ∈ all ∧ x ∉ source}).
*
* Obsolete Messages: also called 'obsolete' (obsolete = source ∖ all = {x: x ∈ source ∧ x ∉ all}).
*
* Result: also called 'result', is the resulting catalogue for the given domain that holds the same messages as 'all'.
*
* @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
*/
interface OperationInterface
Expand All @@ -28,7 +42,7 @@ interface OperationInterface
public function getDomains();

/**
* Returns all valid messages after operation.
* Returns all valid messages ('all') after operation.
*
* @param string $domain
*
Expand All @@ -37,7 +51,7 @@ public function getDomains();
public function getMessages($domain);

/**
* Returns new messages after operation.
* Returns new messages ('new') after operation.
*
* @param string $domain
*
Expand All @@ -46,7 +60,7 @@ public function getMessages($domain);
public function getNewMessages($domain);

/**
* Returns obsolete messages after operation.
* Returns obsolete messages ('obsolete') after operation.
*
* @param string $domain
*
Expand All @@ -55,7 +69,7 @@ public function getNewMessages($domain);
public function getObsoleteMessages($domain);

/**
* Returns resulting catalogue.
* Returns resulting catalogue ('result').
*
* @return MessageCatalogueInterface
*/
Expand Down
69 changes: 69 additions & 0 deletions src/Symfony/Component/Translation/Catalogue/TargetOperation.php
@@ -0,0 +1,69 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Translation\Catalogue;

/**
* Target operation between two catalogues:
* intersection = source ∩ target = {x: x ∈ source ∧ x ∈ target}
* all = intersection ∪ (target ∖ intersection) = target
* new = all ∖ source = {x: x ∈ target ∧ x ∉ source}
* obsolete = source ∖ all = source ∖ target = {x: x ∈ source ∧ x ∉ target}
* Basically, the result contains messages from the target catalogue.
*
* @author Michael Lee <michael.lee@zerustech.com>
*/
class TargetOperation extends AbstractOperation
{
/**
* {@inheritdoc}
*/
protected function processDomain($domain)
{
$this->messages[$domain] = array(
'all' => array(),
'new' => array(),
'obsolete' => array(),
);

// For 'all' messages, the code can't be simplified as ``$this->messages[$domain]['all'] = $target->all($domain);``,
// because doing so will drop messages like {x: x ∈ source ∧ x ∉ target.all ∧ x ∈ target.fallback}
//
// For 'new' messages, the code can't be simplied as ``array_diff_assoc($this->target->all($domain), $this->source->all($domain));``
// because doing so will not exclude messages like {x: x ∈ target ∧ x ∉ source.all ∧ x ∈ source.fallback}
//
// For 'obsolete' messages, the code can't be simplifed as ``array_diff_assoc($this->source->all($domain), $this->target->all($domain))``
// because doing so will not exclude messages like {x: x ∈ source ∧ x ∉ target.all ∧ x ∈ target.fallback}

foreach ($this->source->all($domain) as $id => $message) {
if ($this->target->has($id, $domain)) {
$this->messages[$domain]['all'][$id] = $message;
$this->result->add(array($id => $message), $domain);
if (null !== $keyMetadata = $this->source->getMetadata($id, $domain)) {
$this->result->setMetadata($id, $keyMetadata, $domain);
}
} else {
$this->messages[$domain]['obsolete'][$id] = $message;
}
}

foreach ($this->target->all($domain) as $id => $message) {
if (!$this->source->has($id, $domain)) {
$this->messages[$domain]['all'][$id] = $message;
$this->messages[$domain]['new'][$id] = $message;
$this->result->add(array($id => $message), $domain);
if (null !== $keyMetadata = $this->target->getMetadata($id, $domain)) {
$this->result->setMetadata($id, $keyMetadata, $domain);
}
}
}
}
}
Expand Up @@ -12,69 +12,13 @@
namespace Symfony\Component\Translation\Tests\Catalogue;

use Symfony\Component\Translation\Catalogue\DiffOperation;
use Symfony\Component\Translation\MessageCatalogue;
use Symfony\Component\Translation\MessageCatalogueInterface;

class DiffOperationTest extends AbstractOperationTest
/**
* @group legacy
*/
class DiffOperationTest extends TargetOperationTest
{
public function testGetMessagesFromSingleDomain()
{
$operation = $this->createOperation(
new MessageCatalogue('en', array('messages' => array('a' => 'old_a', 'b' => 'old_b'))),
new MessageCatalogue('en', array('messages' => array('a' => 'new_a', 'c' => 'new_c')))
);

$this->assertEquals(
array('a' => 'old_a', 'c' => 'new_c'),
$operation->getMessages('messages')
);

$this->assertEquals(
array('c' => 'new_c'),
$operation->getNewMessages('messages')
);

$this->assertEquals(
array('b' => 'old_b'),
$operation->getObsoleteMessages('messages')
);
}

public function testGetResultFromSingleDomain()
{
$this->assertEquals(
new MessageCatalogue('en', array(
'messages' => array('a' => 'old_a', 'c' => 'new_c'),
)),
$this->createOperation(
new MessageCatalogue('en', array('messages' => array('a' => 'old_a', 'b' => 'old_b'))),
new MessageCatalogue('en', array('messages' => array('a' => 'new_a', 'c' => 'new_c')))
)->getResult()
);
}

public function testGetResultWithMetadata()
{
$leftCatalogue = new MessageCatalogue('en', array('messages' => array('a' => 'old_a', 'b' => 'old_b')));
$leftCatalogue->setMetadata('a', 'foo', 'messages');
$leftCatalogue->setMetadata('b', 'bar', 'messages');
$rightCatalogue = new MessageCatalogue('en', array('messages' => array('b' => 'new_b', 'c' => 'new_c')));
$rightCatalogue->setMetadata('b', 'baz', 'messages');
$rightCatalogue->setMetadata('c', 'qux', 'messages');

$diffCatalogue = new MessageCatalogue('en', array('messages' => array('b' => 'old_b', 'c' => 'new_c')));
$diffCatalogue->setMetadata('b', 'bar', 'messages');
$diffCatalogue->setMetadata('c', 'qux', 'messages');

$this->assertEquals(
$diffCatalogue,
$this->createOperation(
$leftCatalogue,
$rightCatalogue
)->getResult()
);
}

protected function createOperation(MessageCatalogueInterface $source, MessageCatalogueInterface $target)
{
return new DiffOperation($source, $target);
Expand Down

0 comments on commit 353c94d

Please sign in to comment.