[translation][framework-bundle] Deprecated DiffOperation

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

Author: zerustech

CHANGELOG.md

@@ -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
 -----

Catalogue/AbstractOperation.php

@@ -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 <[email protected]>
  */
 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
      */
@@ -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);
 }

Catalogue/DiffOperation.php

@@ -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 <[email protected]>
+ *
+ * @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);
-                }
-            }
-        }
-    }
 }

Catalogue/MergeOperation.php

@@ -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 <[email protected]>
  */

Catalogue/OperationInterface.php

@@ -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 <[email protected]>
  */
 interface OperationInterface
@@ -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
      *
@@ -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
      *
@@ -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
      *
@@ -55,7 +69,7 @@ public function getNewMessages($domain);
     public function getObsoleteMessages($domain);
 
     /**
-     * Returns resulting catalogue.
+     * Returns resulting catalogue ('result').
      *
      * @return MessageCatalogueInterface
      */

Catalogue/TargetOperation.php

@@ -0,0 +1,69 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * 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 <[email protected]>
+ */
+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);
+                }
+            }
+        }
+    }
+}

Tests/Catalogue/DiffOperationTest.php

@@ -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);