Skip to content
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.

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

added support for xml-attributes as discriminators #692

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
62 changes: 62 additions & 0 deletions doc/reference/annotations.rst
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,68 @@ Resulting XML:
<name><![CDATA[Johannes]]></name>
</result>


@XmlDiscriminator
~~~~~~~~~~~~~~~~~
This annotation allows to modify the behaviour of @Discriminator regarding handling of XML.


Available Options:

+-------------------------------------+--------------------------------------------------+
| Type | Description |
+=====================================+==================================================+
| attribute | use an attribute instead of a child node |
+-------------------------------------+--------------------------------------------------+
| cdata | render child node content with or without cdata |
+-------------------------------------+--------------------------------------------------+

Example for "attribute":
.. code-block :: php

<?php

use JMS\Serializer\Annotation\Discriminator;
use JMS\Serializer\Annotation\XmlDiscriminator;

/**
* @Discriminator(field = "type", map = {"car": "Car", "moped": "Moped"}, groups={"foo", "bar"})
* @XmlDiscriminator(attribute=true)
*/
abstract class Vehicle { }
class Car extends Vehicle { }

Resulting XML:

.. code-block :: xml

<vehicle type="car" />


Example for "cdata":

.. code-block :: php
<?php

use JMS\Serializer\Annotation\Discriminator;
use JMS\Serializer\Annotation\XmlDiscriminator;



/**
* @Discriminator(field = "type", map = {"car": "Car", "moped": "Moped"}, groups={"foo", "bar"})
* @XmlDiscriminator(attribute=true)
*/
abstract class Vehicle { }
class Car extends Vehicle { }

Resulting XML:

.. code-block :: xml

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


@XmlValue
~~~~~~~~~
This allows you to mark properties which should be set as the value of the
Expand Down
1 change: 1 addition & 0 deletions doc/reference/xml_reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ XML Reference
accessor-order="custom" custom-accessor-order="propertyName1,propertyName2,...,propertyNameN"
access-type="public_method" discriminator-field-name="type" read-only="false">
<xml-namespace prefix="atom" uri="http://www.w3.org/2005/Atom"/>
<xml-discriminator attribute="true" cdata="false"/>
<discriminator-class value="some-value">ClassName</discriminator-class>
<discriminator-groups>
<group>foo</group>
Expand Down
3 changes: 3 additions & 0 deletions doc/reference/yml_reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ YAML Reference
exclusion_policy: ALL
xml_root_name: foobar
xml_root_namespace: http://your.default.namespace
xml_discriminator:
attribute: true
cdata: false
exclude: true
read_only: false
access_type: public_method # defaults to property
Expand Down
37 changes: 37 additions & 0 deletions src/JMS/Serializer/Annotation/XmlDiscriminator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

/*
* Copyright 2016 Björn Bösel <[email protected]>
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace JMS\Serializer\Annotation;


/**
* @Annotation
* @Target("CLASS")
*/
class XmlDiscriminator
{
/**
* @var boolean
*/
public $attribute = false;

/**
* @var boolean
*/
public $cdata = true;
}
5 changes: 5 additions & 0 deletions src/JMS/Serializer/GraphNavigator.php
Original file line number Diff line number Diff line change
Expand Up @@ -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]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

i will put this statement before the previous one

$typeValue = (string) $data[$metadata->discriminatorFieldName];
break;

default:
throw new \LogicException(sprintf(
'The discriminator field name "%s" for base-class "%s" was not found in input data.',
Expand Down
14 changes: 14 additions & 0 deletions src/JMS/Serializer/Metadata/ClassMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ class ClassMetadata extends MergeableClassMetadata
public $discriminatorMap = array();
public $discriminatorGroups = array();

public $xmlDiscriminatorAttribute = false;
public $xmlDiscriminatorCData = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be $xmlDiscriminatorCDATA or $xmlDiscriminatorCdata ?

Copy link

Choose a reason for hiding this comment

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

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


public function setDiscriminator($fieldName, array $map, array $groups = array())
{
if (empty($fieldName)) {
Expand Down Expand Up @@ -203,6 +206,7 @@ public function merge(MergeableInterface $object)
$this->discriminatorGroups
);
$discriminatorProperty->serializedName = $this->discriminatorFieldName;
$discriminatorProperty->xmlAttribute = $this->xmlDiscriminatorAttribute;
$this->propertyMetadata[$this->discriminatorFieldName] = $discriminatorProperty;
}

Expand Down Expand Up @@ -249,6 +253,8 @@ public function serialize()
$this->discriminatorGroups,
parent::serialize(),
'discriminatorGroups' => $this->discriminatorGroups,
'xmlDiscriminatorAttribute' => $this->xmlDiscriminatorAttribute,
'xmlDiscriminatorCData' => $this->xmlDiscriminatorCData,
));
}

Expand Down Expand Up @@ -279,6 +285,14 @@ public function unserialize($str)
$this->discriminatorGroups = $deserializedData['discriminatorGroups'];
}

if (isset($deserializedData['xmlDiscriminatorAttribute'])) {
$this->xmlDiscriminatorAttribute = $deserializedData['xmlDiscriminatorAttribute'];
}

if (isset($deserializedData['xmlDiscriminatorCData'])) {
$this->xmlDiscriminatorCData = $deserializedData['xmlDiscriminatorCData'];
}

parent::unserialize($parentStr);
}

Expand Down
4 changes: 4 additions & 0 deletions src/JMS/Serializer/Metadata/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
namespace JMS\Serializer\Metadata\Driver;

use JMS\Serializer\Annotation\Discriminator;
use JMS\Serializer\Annotation\XmlDiscriminator;
use JMS\Serializer\GraphNavigator;
use JMS\Serializer\Annotation\HandlerCallback;
use JMS\Serializer\Annotation\AccessorOrder;
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

cast to boolean please

$classMetadata->xmlDiscriminatorCData = $annot->cdata;
Copy link
Collaborator

Choose a reason for hiding this comment

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

cast to boolean please

}
}

Expand Down
9 changes: 9 additions & 0 deletions src/JMS/Serializer/Metadata/Driver/XmlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,15 @@ protected function loadMetadataFromFile(\ReflectionClass $class, $path)
$metadata->registerNamespace((string) $xmlNamespace->attributes()->uri, $prefix);
}

foreach ($elem->xpath('./xml-discriminator') as $xmlDiscriminator) {
if (isset($xmlDiscriminator->attributes()->attribute)) {
$metadata->xmlDiscriminatorAttribute = (string) $xmlDiscriminator->attributes()->attribute === 'true';
}
if (isset($xmlDiscriminator->attributes()->cdata)) {
$metadata->xmlDiscriminatorCData = (string) $xmlDiscriminator->attributes()->cdata === 'true';
}
}

foreach ($elem->xpath('./virtual-property') as $method) {
if ( ! isset($method->attributes()->method)) {
throw new RuntimeException('The method attribute must be set for all virtual-property elements.');
Expand Down
9 changes: 9 additions & 0 deletions src/JMS/Serializer/Metadata/Driver/YamlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

cast to boolean please

}
if (isset($config['xml_discriminator']['cdata'])) {
$metadata->xmlDiscriminatorCData = $config['xml_discriminator']['cdata'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

cast to boolean please

}
}

if (array_key_exists('xml_namespaces', $config)) {

foreach ($config['xml_namespaces'] as $prefix => $uri) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

/*
* Copyright 2016 Björn Bösel <[email protected]>
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace JMS\Serializer\Tests\Fixtures\Discriminator;

class ObjectWithXmlAttributeDiscriminatorChild extends ObjectWithXmlAttributeDiscriminatorParent
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

/*
* Copyright 2016 Björn Bösel <[email protected]>
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace JMS\Serializer\Tests\Fixtures\Discriminator;

use JMS\Serializer\Annotation as Serializer;

/**
* @Serializer\Discriminator(field = "type", map = {
* "child": "JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild"
* })
* @Serializer\XmlDiscriminator(attribute=true, cdata=false)
*/
class ObjectWithXmlAttributeDiscriminatorParent
{

}
20 changes: 20 additions & 0 deletions tests/JMS/Serializer/Tests/Metadata/Driver/BaseDriverTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
use JMS\Serializer\Metadata\ClassMetadata;
use JMS\Serializer\Metadata\PropertyMetadata;
use JMS\Serializer\Metadata\VirtualPropertyMetadata;
use JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild;
use JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorParent;
use Metadata\Driver\DriverInterface;

abstract class BaseDriverTest extends \PHPUnit_Framework_TestCase
Expand Down Expand Up @@ -171,6 +173,24 @@ public function testLoadDiscriminator()
);
}

public function testLoadXmlDiscriminator()
{
/** @var $m ClassMetadata */
$m = $this->getDriver()->loadMetadataForClass(new \ReflectionClass(ObjectWithXmlAttributeDiscriminatorParent::class));

$this->assertNotNull($m);
$this->assertEquals('type', $m->discriminatorFieldName);
$this->assertEquals($m->name, $m->discriminatorBaseClass);
$this->assertEquals(
array(
'child' => ObjectWithXmlAttributeDiscriminatorChild::class,
),
$m->discriminatorMap
);
$this->assertTrue($m->xmlDiscriminatorAttribute);
$this->assertFalse($m->xmlDiscriminatorCData);
}

public function testLoadDiscriminatorWithGroup()
{
/** @var $m ClassMetadata */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

use JMS\Serializer\Metadata\ClassMetadata;
use JMS\Serializer\Metadata\PropertyMetadata;

$metadata = new ClassMetadata('JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorParent');
$metadata->setDiscriminator('type', array(
'child' => 'JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild'
));
$metadata->xmlDiscriminatorAttribute = true;
$metadata->xmlDiscriminatorCData = false;
return $metadata;
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<serializer>
<class name="JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorParent"
discriminator-field-name="type"
xml-discriminator-attribute="true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

sorry, forgot to remove that one...

>
<discriminator-class value="child">JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild</discriminator-class>
<xml-discriminator attribute="true" cdata="false" />
</class>
</serializer>
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorParent:
discriminator:
field_name: type
map:
child: JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild
xml_discriminator:
attribute: true
cdata: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

16 changes: 16 additions & 0 deletions tests/JMS/Serializer/Tests/Serializer/XmlSerializationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use JMS\Serializer\Naming\SerializedNameAnnotationStrategy;
use JMS\Serializer\SerializationContext;
use JMS\Serializer\Serializer;
use JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorParent;
use JMS\Serializer\Tests\Fixtures\InvalidUsageOfXmlValue;
use JMS\Serializer\Exception\InvalidArgumentException;
use JMS\Serializer\Tests\Fixtures\PersonCollection;
Expand All @@ -40,6 +41,7 @@
use JMS\Serializer\Tests\Fixtures\ObjectWithNamespacesAndList;
use JMS\Serializer\XmlSerializationVisitor;
use PhpCollection\Map;
use JMS\Serializer\Tests\Fixtures\Discriminator\ObjectWithXmlAttributeDiscriminatorChild;

class XmlSerializationTest extends BaseSerializationTest
{
Expand Down Expand Up @@ -361,6 +363,20 @@ public function testWithoutFormatedOutputByXmlSerializationVisitor()
$this->assertXmlStringEqualsXmlString($this->getContent('simple_class_object_minified'), $stringXml);
}

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

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.


$this->assertInstanceOf(
ObjectWithXmlAttributeDiscriminatorChild::class,
$this->deserialize(
$xml->asXML(),
ObjectWithXmlAttributeDiscriminatorParent::class
)
);
}

private function xpathFirstToString(\SimpleXMLElement $xml, $xpath)
{
$nodes = $xml->xpath($xpath);
Expand Down