added support for xml-attributes as discriminators #692

twtgrowth · 2017-01-05T16:04:39Z

Until now when using Discriminator-maps with XML, the discriminator was hard coded to be a child node. This PR allows to use an attribute instead if desired.

 /**
 * @Discriminator(field = "type", map = {"car": "Car", "moped": "Moped"})
 */
 abstract class Vehicle { }
 class Car extends Vehicle { }
 class Moped extends Vehicle { }
 $serializer->serialize(new Car(), 'xml');

would result in

<vehicle>
    <type>car</type>
</vehicle>

now you can do this:

/**
 * @Discriminator(field = "type", map = {"car": "Car", "moped": "Moped"})
 * @XmlDiscriminatorAttribute
 */
abstract class Vehicle { }
class Car extends Vehicle { }
class Moped extends Vehicle { }

$serializer->serialize(new Car(), 'xml');

to get:

<vehicle type="car" />

goetas · 2017-01-05T16:12:18Z

Thanks for the pull request. Have a look also to #560

Some considerations:

instead of adding a new annotation (that requires a lot of changes...) what about properties of the @Discriminator annotation?
this then opens possibilities as namespaces... and many other things

fivetide · 2017-01-06T09:16:11Z

Actually adding this to Discriminator, just like in #560 was our first approach, but after discussing this internally we concluded Discriminator would be better off remaining format-agnostic. With additional options in mind, i would like to suggest changing the new annotation to

@XmlDiscriminator(attribute=true)

which would enable future changes like

@XmlDiscriminator(cdata=false)
@XmlDiscriminator(namespace="foo/bar")

and changing the other drivers accordingly...

goetas · 2017-01-06T09:21:14Z

hmm.. sounds reasonable. even now we have almost always xml-specific annotations.

So interesting options would be:

@XmlDiscriminator(
  attribute=true, // default = false
  cdata=false, // default = true
  namespace="http://xxx"  // default = NULL
)

right?

fivetide · 2017-01-06T09:25:00Z

Correct, just one question: I belive the current behaviour is to use cdata, which means we should default cdata to true...?

goetas · 2017-01-06T09:27:11Z

@fivetide you are right, my bad, i have updated my comment

fivetide · 2017-01-06T09:27:52Z

Okay, i will propably get that done on monday, will update the PR then :)

fivetide · 2017-01-09T09:02:51Z

Ok, i updated the PR with the changes we agreed on.

goetas

Good job :)
i will just change the YAML format and few minor changes here and there...

the namespaced xml element will be cool to have, but can be implemented in a second iteration...)

goetas · 2017-01-09T10:04:56Z

src/JMS/Serializer/Metadata/ClassMetadata.php

@@ -58,6 +58,9 @@ class ClassMetadata extends MergeableClassMetadata
    public $discriminatorMap = array();
    public $discriminatorGroups = array();

+    public $xmlDiscriminatorAttribute = false;
+    public $xmlDiscriminatorCData = true;


Should it be $xmlDiscriminatorCDATA or $xmlDiscriminatorCdata ?

capitalizing CD follows the convention used in PropertyMetadata::$xmlElementCData ...

goetas · 2017-01-09T10:08:17Z

src/JMS/Serializer/Metadata/Driver/YamlDriver.php

@@ -257,6 +257,15 @@ private function addClassProperties(ClassMetadata $metadata, array $config)
            $metadata->xmlRootNamespace = (string) $config['xml_root_namespace'];
        }

+        if (isset($config['xml_discriminator'])) {
+            if (isset($config['xml_discriminator']['attribute'])) {
+                $metadata->xmlDiscriminatorAttribute = $config['xml_discriminator']['attribute'];


cast to boolean please

goetas · 2017-01-09T10:08:25Z

src/JMS/Serializer/Metadata/Driver/YamlDriver.php

+                $metadata->xmlDiscriminatorAttribute = $config['xml_discriminator']['attribute'];
+            }
+            if (isset($config['xml_discriminator']['cdata'])) {
+                $metadata->xmlDiscriminatorCData = $config['xml_discriminator']['cdata'];


cast to boolean please

goetas · 2017-01-09T10:08:35Z

src/JMS/Serializer/Metadata/Driver/AnnotationDriver.php

@@ -99,6 +100,9 @@ public function loadMetadataForClass(\ReflectionClass $class)
                } else {
                    $classMetadata->setDiscriminator($annot->field, $annot->map, $annot->groups);
                }
+            } elseif ($annot instanceof XmlDiscriminator) {
+                $classMetadata->xmlDiscriminatorAttribute = $annot->attribute;


cast to boolean please

goetas · 2017-01-09T10:08:39Z

src/JMS/Serializer/Metadata/Driver/AnnotationDriver.php

@@ -99,6 +100,9 @@ public function loadMetadataForClass(\ReflectionClass $class)
                } else {
                    $classMetadata->setDiscriminator($annot->field, $annot->map, $annot->groups);
                }
+            } elseif ($annot instanceof XmlDiscriminator) {
+                $classMetadata->xmlDiscriminatorAttribute = $annot->attribute;
+                $classMetadata->xmlDiscriminatorCData = $annot->cdata;


cast to boolean please

goetas · 2017-01-09T10:11:05Z

...alizer/Tests/Metadata/Driver/xml/Discriminator.ObjectWithXmlAttributeDiscriminatorParent.xml

+<serializer>
+    <class name="JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorParent"
+           discriminator-field-name="type"
+           xml-discriminator-attribute="true"


what is this attribute (i mean xml-discriminator-attribute)?

sorry, forgot to remove that one...

goetas · 2017-01-09T10:15:55Z

...alizer/Tests/Metadata/Driver/yml/Discriminator.ObjectWithXmlAttributeDiscriminatorParent.yml

+            child: JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild
+    xml_discriminator:
+        attribute: true
+        cdata: false


what about having a structure as:

JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorParent: discriminator: field_name: type map: child: JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild xml_attribute: true xml_element: cdata: false namespace: http://www.w3.org/2005/Atom

this is not a big deal, but is just to keep consistency with the current YAML metadata format.
(see http://jmsyst.com/libs/serializer/master/reference/yml_reference the xml_attribute property example)

Having the YAML format being wildly different to annotations and xml feels odd to me, but if you insist, i will change it...

My idea is just to keep it consistent. I would like to keep consistency within a format mainly.
Having consistency across formats is nice to have.

Since XML and YAML formats have already differences, and users use only one of them, will try to keep them consistent with the rest of the format itself.

goetas · 2017-01-09T10:21:46Z

tests/JMS/Serializer/Tests/Serializer/XmlSerializationTest.php

+    public function testDiscriminatorAsXmlAttribute()
+    {
+        $xml = simplexml_load_string($this->serialize(new ObjectWithXmlAttributeDiscriminatorChild()));
+        $this->assertEquals('type="child"', trim($xml->xpath('//result/@type')[0]->saveXML()));


instead of "re-loading" the xml into a simplexml object, why not re-use the $this->getContent('template_name') method? the whole test case will be:

$xml = $this->serialize(new ObjectWithXmlAttributeDiscriminatorChild()); $this->assertEquals($this->getContent('xml_discriminator_attribute'), $xml); $this->assertInstanceOf( ObjectWithXmlAttributeDiscriminatorChild::class, $this->deserialize( $xml, ObjectWithXmlAttributeDiscriminatorParent::class ) );

What would be the benefit when comparing to a predefined xml? I assumed the test should only cover if the discriminator is rendered as an attribute, while other aspects of xml-generation (whitespace, xml-header, if the tag is rendered as self-closing...) should not break the test, which might happen when comparing to predefined xml..

This is an interesting topic about testing, and how much we want to do "unit" testing...

Your test currently tests only of the type is rendered as attribute... but what if for some other change, make the type to be rendered using both strategies.. I mean as attribute and as element? This test will not detect it.

Of course, testing the whole xml content, has some drawbacks, as example will give some "false" failures if we decide to change indentation or other format specific differences... that generally should not make fail your testcase.

To be honest, I prefer to have a false failure, instead of having a not detected failure.

(this means less "unit" testing, and going more in direction of functional or integration tests... (event if the border between this definitions is really blurry)

What is your opinion on it?

Personally and totally subjective, i like to have my tests represent actual requirements and i like to be able to treat them as much. When working with foreign code, i can not determine of a false failure is indeed false, or if there is an actual requirement... But since in this case, it is pretty obvious, i feel okay doing it your way.

goetas · 2017-01-09T10:30:42Z

src/JMS/Serializer/GraphNavigator.php

@@ -261,6 +261,11 @@ private function resolveMetadata($data, ClassMetadata $metadata)
                $typeValue = (string) $data->{$metadata->discriminatorFieldName};
                break;

+            // Check XML attribute for discriminatorFieldName
+            case is_object($data) && $metadata->xmlDiscriminatorAttribute && isset($data[$metadata->discriminatorFieldName]):


i will put this statement before the previous one

removed old attribute from test fixture changed order of metadata-resolution

changed syntax of yml configuration

goetas

Sorry missed that small PSR thing. Will try to include this in 1.5.0 (next feature release).

goetas · 2017-01-09T17:01:31Z

src/JMS/Serializer/Metadata/Driver/YamlDriver.php

@@ -278,6 +278,17 @@ private function addClassProperties(ClassMetadata $metadata, array $config)
                }
                $groups = isset($config['discriminator']['groups']) ? $config['discriminator']['groups'] : array();
                $metadata->setDiscriminator($config['discriminator']['field_name'], $config['discriminator']['map'], $groups);
+


PSR standards, max one empty line

fivetide · 2017-01-11T08:53:41Z

Thanks a lot! 👍

added support for xml-attributes as discriminators

14fadd2

goetas self-assigned this Jan 6, 2017

goetas added the WIP label Jan 6, 2017

Björn Bösel added 2 commits January 9, 2017 09:06

Changed @XmlDiscriminatorAttribute to @Discriminator(attribute=true)

9ba0de2

Added "cdata" option to @XmlDiscriminator

5d63f75

goetas requested changes Jan 9, 2017

View reviewed changes

Björn Bösel added 2 commits January 9, 2017 12:23

added typecasts

8d4548d

removed old attribute from test fixture changed order of metadata-resolution

changed testing method to comparison with prerendered xml

23348ad

changed syntax of yml configuration

goetas added this to the v1.5 milestone Jan 9, 2017

goetas approved these changes Jan 9, 2017

View reviewed changes

goetas added To Merge and removed WIP labels Jan 9, 2017

goetas reviewed Jan 9, 2017

View reviewed changes

removed extra empty line

a1f4e04

goetas merged commit a1f4e04 into schmittjoh:master Jan 10, 2017

goetas mentioned this pull request Jan 16, 2017

Add support for XML Stylesheets #489

Open

goetas added the enhancement label Jan 19, 2017

This was referenced Apr 24, 2017

Discriminator should support xmlAttribute #547

Closed

Fix for #559 to enable @discriminator fields to not be serialized as CDATA #560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for xml-attributes as discriminators #692

added support for xml-attributes as discriminators #692

twtgrowth commented Jan 5, 2017

goetas commented Jan 5, 2017

fivetide commented Jan 6, 2017

goetas commented Jan 6, 2017 •

edited

Loading

fivetide commented Jan 6, 2017

goetas commented Jan 6, 2017

fivetide commented Jan 6, 2017

fivetide commented Jan 9, 2017

goetas left a comment

goetas Jan 9, 2017

fivetide Jan 9, 2017

goetas Jan 9, 2017

goetas Jan 9, 2017

goetas Jan 9, 2017

goetas Jan 9, 2017

goetas Jan 9, 2017

fivetide Jan 9, 2017

goetas Jan 9, 2017

fivetide Jan 9, 2017

goetas Jan 9, 2017

goetas Jan 9, 2017

fivetide Jan 9, 2017

goetas Jan 9, 2017

fivetide Jan 9, 2017

goetas Jan 9, 2017

goetas left a comment

goetas Jan 9, 2017

fivetide commented Jan 11, 2017

added support for xml-attributes as discriminators #692

added support for xml-attributes as discriminators #692

Conversation

twtgrowth commented Jan 5, 2017

goetas commented Jan 5, 2017

fivetide commented Jan 6, 2017

goetas commented Jan 6, 2017 • edited Loading

fivetide commented Jan 6, 2017

goetas commented Jan 6, 2017

fivetide commented Jan 6, 2017

fivetide commented Jan 9, 2017

goetas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetide commented Jan 11, 2017

goetas commented Jan 6, 2017 •

edited

Loading