Add deprecations for ID attribute in XML mappings (doctrine#1807)

alcaeus · Jan 10, 2019 · 2add05f · 2add05f
1 parent 46b182b
commit 2add05f
Show file tree

Hide file tree

Showing 10 changed files with 117 additions and 27 deletions.
diff --git a/UPGRADE-1.3.md b/UPGRADE-1.3.md
@@ -9,3 +9,9 @@ switch to using annotations or XML mapping files. The following classes have
 been deprecated:
  * `Doctrine\ODM\MongoDB\Mapping\Driver\YamlDriver`
  * `Doctrine\ODM\MongoDB\Mapping\Driver\SimplifiedYamlDriver`
+
+### `id` attribute in XML mappings deprecated
+
+The `id` attribute to denote identifiers in XML mappings has been deprecated and
+will be removed in 2.0. Instead, use the new `id` element to map identifiers
+instead of mapping them with `field`.
diff --git a/docs/en/reference/basic-mapping.rst b/docs/en/reference/basic-mapping.rst
@@ -247,7 +247,7 @@ Here is an example:
                         xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
                         http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
           <document name="Documents\User">
-                <field fieldName="id" id="true" />
+                <id />
           </document>
         </doctrine-mongo-mapping>
 
@@ -299,7 +299,7 @@ Here is an example how to manually set a string identifier for your documents:
                                                     http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
     
             <document name="MyPersistentClass">
-                <field name="id" id="true" strategy="NONE" type="string" />
+                <id strategy="NONE" type="string" />
             </document>
         </doctrine-mongo-mapping>
     
@@ -367,9 +367,9 @@ as an option for the ``CUSTOM`` strategy:
                                                     http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
 
             <document name="MyPersistentClass">
-                <field name="id" id="true" strategy="CUSTOM" type="string">
-                    <id-generator-option name="class" value="Vendor\Specific\Generator" />
-                </field>
+                <id strategy="CUSTOM" type="string">
+                    <generator-option name="class" value="Vendor\Specific\Generator" />
+                </id>
             </document>
         </doctrine-mongo-mapping>
 
@@ -423,7 +423,7 @@ Example:
                         xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
                         http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
           <document name="Documents\User">
-                <field fieldName="id" id="true" />
+                <id />
                 <field fieldName="username" type="string" />
           </document>
         </doctrine-mongo-mapping>

diff --git a/docs/en/reference/capped-collections.rst b/docs/en/reference/capped-collections.rst
@@ -47,7 +47,7 @@ the ``@Document`` annotation:
                           xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
                           http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
             <document name="Documents\Category" collection="collname" capped-collection="true" capped-collection-size="100000" capped-collection-max="1000">
-                <field fieldName="id" id="true" />
+                <id />
                 <field fieldName="name" type="string" />
             </document>
         </doctrine-mongo-mapping>

diff --git a/docs/en/reference/indexes.rst b/docs/en/reference/indexes.rst
@@ -480,7 +480,7 @@ make it to the database and cause performance problems.
                             http://doctrine-project.org/schemas/orm/doctrine-mongo-mapping.xsd">
         
             <document name="Documents\Place" require-indexes="true">
-                <field fieldName="id" id="true" />
+                <id />
                 <field fieldName="city" type="string" />
                 <indexes>
                     <index>

diff --git a/docs/en/reference/xml-mapping.rst b/docs/en/reference/xml-mapping.rst
@@ -110,7 +110,7 @@ of several common elements:
                         http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
     
         <document name="Documents\User" db="documents" collection="users">
-            <field fieldName="id" id="true" />
+            <id />
             <field fieldName="username" name="login" type="string" />
             <field fieldName="email" type="string" unique="true" order="desc" />
             <field fieldName="createdAt" type="date" />

diff --git a/docs/en/tutorials/getting-started.rst b/docs/en/tutorials/getting-started.rst
@@ -114,7 +114,7 @@ You can provide your mapping information in Annotations, XML, or YAML:
                         xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
                         http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
           <document name="Documents\User">
-                <field fieldName="id" id="true" />
+                <id />
                 <field fieldName="name" type="string" />
                 <field fieldName="email" type="string" />
                 <reference-many fieldName="posts" targetDocument="Documents\BlogPost">
@@ -131,7 +131,7 @@ You can provide your mapping information in Annotations, XML, or YAML:
                         xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
                         http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
           <document name="Documents\BlogPost">
-                <field fieldName="id" id="true" />
+                <id />
                 <field fieldName="title" type="string" />
                 <field fieldName="body" type="string" />
                 <field fieldName="createdAt" type="date" />

diff --git a/doctrine-mongo-mapping.xsd b/doctrine-mongo-mapping.xsd
@@ -37,6 +37,7 @@
 
   <xs:complexType name="document">
     <xs:sequence>
+      <xs:element name="id" type="odm:id" minOccurs="0" maxOccurs="1"/>
       <xs:element name="field" type="odm:field" minOccurs="0" maxOccurs="unbounded"/>
       <xs:element name="embed-one" type="odm:embed-one" minOccurs="0" maxOccurs="unbounded"/>
       <xs:element name="embed-many" type="odm:embed-many" minOccurs="0" maxOccurs="unbounded"/>
@@ -74,9 +75,11 @@
   </xs:complexType>
 
   <xs:complexType name="field">
+    <!-- deprecated -->
     <xs:sequence>
       <xs:element name="id-generator-option" type="odm:id-generator-option" minOccurs="0" maxOccurs="unbounded" />
     </xs:sequence>
+    <!-- deprecated -->
     <xs:attribute name="id" type="xs:boolean" default="false" />
     <xs:attribute name="name" type="xs:NMTOKEN" />
     <xs:attribute name="type" type="xs:NMTOKEN" />
@@ -102,6 +105,15 @@
     <xs:attribute name="unique" type="xs:boolean" />
   </xs:complexType>
 
+  <xs:complexType name="id">
+    <xs:choice minOccurs="0" maxOccurs="unbounded">
+      <xs:element name="generator-option" type="odm:id-generator-option" minOccurs="0" maxOccurs="unbounded" />
+    </xs:choice>
+    <xs:attribute name="type" type="xs:NMTOKEN" />
+    <xs:attribute name="strategy" type="xs:NMTOKEN" default="auto" />
+    <xs:attribute name="field-name" type="xs:NMTOKEN" default="id" />
+  </xs:complexType>
+
   <xs:complexType name="id-generator-option">
     <xs:attribute name="name" type="xs:NMTOKEN" use="required"/>
     <xs:attribute name="value" type="xs:string" use="required"/>

diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php b/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php
@@ -24,6 +24,10 @@
 use Doctrine\ODM\MongoDB\Mapping\ClassMetadata as MappingClassMetadata;
 use Doctrine\ODM\MongoDB\Utility\CollectionHelper;
 use Doctrine\ODM\MongoDB\Mapping\ClassMetadataInfo;
+use SimpleXMLElement;
+use const E_USER_DEPRECATED;
+use function sprintf;
+use function trigger_error;
 
 /**
  * XmlDriver is a metadata driver that enables mapping through XML files.
@@ -48,7 +52,7 @@ public function __construct($locator, $fileExtension = self::DEFAULT_FILE_EXTENS
     public function loadMetadataForClass($className, ClassMetadata $class)
     {
         /* @var $class ClassMetadataInfo */
-        /* @var $xmlRoot \SimpleXMLElement */
+        /* @var $xmlRoot SimpleXMLElement */
         $xmlRoot = $this->getElement($className);
         if ( ! $xmlRoot) {
             return;
@@ -135,6 +139,37 @@ public function loadMetadataForClass($className, ClassMetadata $class)
         if (isset($xmlRoot->{'read-preference'})) {
             $class->setReadPreference(...$this->transformReadPreference($xmlRoot->{'read-preference'}));
         }
+
+        if (isset($xmlRoot->id)) {
+            $field   = $xmlRoot->id;
+            $mapping = [
+                'id' => true,
+                'fieldName' => 'id',
+            ];
+
+            /** @var SimpleXMLElement $attributes */
+            $attributes = $field->attributes();
+            foreach ($attributes as $key => $value) {
+                $mapping[$key] = (string) $value;
+            }
+
+            if (isset($mapping['strategy'])) {
+                $mapping['options'] = [];
+                if (isset($field->{'generator-option'})) {
+                    foreach ($field->{'generator-option'} as $generatorOptions) {
+                        $attributesGenerator = iterator_to_array($generatorOptions->attributes());
+                        if (!isset($attributesGenerator['name']) || !isset($attributesGenerator['value'])) {
+                            continue;
+                        }
+
+                        $mapping['options'][(string)$attributesGenerator['name']] = (string)$attributesGenerator['value'];
+                    }
+                }
+            }
+
+            $this->addFieldMapping($class, $mapping);
+        }
+
         if (isset($xmlRoot->field)) {
             foreach ($xmlRoot->field as $field) {
                 $mapping = array();
@@ -146,13 +181,21 @@ public function loadMetadataForClass($className, ClassMetadata $class)
                         $mapping[$key] = ('true' === $mapping[$key]);
                     }
                 }
-                if (isset($mapping['id']) && $mapping['id'] === true && isset($mapping['strategy'])) {
-                    $mapping['options'] = array();
-                    if (isset($field->{'id-generator-option'})) {
-                        foreach ($field->{'id-generator-option'} as $generatorOptions) {
-                            $attributesGenerator = iterator_to_array($generatorOptions->attributes());
-                            if (isset($attributesGenerator['name']) && isset($attributesGenerator['value'])) {
-                                $mapping['options'][(string) $attributesGenerator['name']] = (string) $attributesGenerator['value'];
+
+
+                if (isset($mapping['id']) && $mapping['id'] === true) {
+                    @trigger_error(sprintf('Using the "id" attribute to denote identifiers in the XML mapping for class "%s" is deprecated and will be removed in 2.0. Please map your identifiers using the "id" element.', $class->getName()), E_USER_DEPRECATED);
+
+                    if (isset($mapping['strategy'])) {
+                        $mapping['options'] = array();
+                        if (isset($field->{'id-generator-option'})) {
+                            foreach ($field->{'id-generator-option'} as $generatorOptions) {
+                                $attributesGenerator = iterator_to_array($generatorOptions->attributes());
+                                if (!isset($attributesGenerator['name']) || !isset($attributesGenerator['value'])) {
+                                    continue;
+                                }
+
+                                $mapping['options'][(string)$attributesGenerator['name']] = (string)$attributesGenerator['value'];
                             }
                         }
                     }
@@ -355,7 +398,7 @@ private function addReferenceMapping(ClassMetadataInfo $class, $reference, $type
         $this->addFieldMapping($class, $mapping);
     }
 
-    private function addIndex(ClassMetadataInfo $class, \SimpleXmlElement $xmlIndex)
+    private function addIndex(ClassMetadataInfo $class, SimpleXmlElement $xmlIndex)
     {
         $attributes = $xmlIndex->attributes();
 
@@ -428,7 +471,7 @@ private function addIndex(ClassMetadataInfo $class, \SimpleXmlElement $xmlIndex)
         $class->addIndex($keys, $options);
     }
 
-    private function getPartialFilterExpression(\SimpleXMLElement $fields)
+    private function getPartialFilterExpression(SimpleXMLElement $fields)
     {
         $partialFilterExpression = [];
         foreach ($fields as $field) {
@@ -463,7 +506,7 @@ private function getPartialFilterExpression(\SimpleXMLElement $fields)
         return $partialFilterExpression;
     }
 
-    private function setShardKey(ClassMetadataInfo $class, \SimpleXmlElement $xmlShardkey)
+    private function setShardKey(ClassMetadataInfo $class, SimpleXmlElement $xmlShardkey)
     {
         $attributes = $xmlShardkey->attributes();
 
@@ -503,10 +546,9 @@ private function setShardKey(ClassMetadataInfo $class, \SimpleXmlElement $xmlSha
      *
      * list($readPreference, $tags) = $this->transformReadPreference($xml->{read-preference});
      *
-     * @param \SimpleXMLElement $xmlReadPreference
      * @return array
      */
-    private function transformReadPreference($xmlReadPreference)
+    private function transformReadPreference(SimpleXMLElement $xmlReadPreference)
     {
         $tags = null;
         if (isset($xmlReadPreference->{'tag-set'})) {

diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/XmlDriverTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/XmlDriverTest.php
@@ -5,6 +5,8 @@
 use Doctrine\ODM\MongoDB\Mapping\ClassMetadataInfo;
 use Doctrine\ODM\MongoDB\Mapping\Driver\XmlDriver;
 use Doctrine\ODM\MongoDB\Mapping\ClassMetadata;
+use TestDocuments\UserCustomIdGenerator;
+use TestDocuments\UserCustomIdGeneratorWithIdField;
 
 class XmlDriverTest extends AbstractDriverTest
 {
@@ -13,10 +15,19 @@ public function setUp()
         $this->driver = new XmlDriver(__DIR__ . '/fixtures/xml');
     }
 
-    public function testDriverShouldReturnOptionsForCustomIdGenerator()
+    public static function getCustomIdGeneratorClasses()
     {
-        $classMetadata = new ClassMetadata('TestDocuments\UserCustomIdGenerator');
-        $this->driver->loadMetadataForClass('TestDocuments\UserCustomIdGenerator', $classMetadata);
+        yield 'legacy-id-attribute' => [UserCustomIdGenerator::class];
+        yield 'id-element' => [UserCustomIdGeneratorWithIdField::class];
+    }
+
+    /**
+     * @dataProvider getCustomIdGeneratorClasses
+     */
+    public function testDriverShouldReturnOptionsForCustomIdGenerator($className)
+    {
+        $classMetadata = new ClassMetadata($className);
+        $this->driver->loadMetadataForClass($className, $classMetadata);
         $this->assertEquals(array(
             'fieldName' => 'id',
             'strategy' => 'custom',
@@ -86,6 +97,11 @@ class UserCustomIdGenerator
     protected $id;
 }
 
+class UserCustomIdGeneratorWithIdField
+{
+    protected $id;
+}
+
 class UserNonStringOptions
 {
     protected $id;

diff --git a/.../Tests/Mapping/Driver/fixtures/xml/TestDocuments.UserCustomIdGeneratorWithIdField.dcm.xml b/.../Tests/Mapping/Driver/fixtures/xml/TestDocuments.UserCustomIdGeneratorWithIdField.dcm.xml
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<doctrine-mongo-mapping xmlns="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping"
+                  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                  xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
+                  http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
+
+    <document name="TestDocuments\UserCustomIdGeneratorWithIdField" db="documents" collection="users">
+        <id strategy="custom">
+            <generator-option name="class" value="TestDocuments\CustomIdGenerator"/>
+            <generator-option name="someOption" value="some-option"/>
+        </id>
+    </document>
+</doctrine-mongo-mapping>