From e4b5d82c9e06c52edfe343c3a2063c5237e2e1a2 Mon Sep 17 00:00:00 2001 From: pbouwdewijn Date: Fri, 19 May 2017 14:10:06 +0200 Subject: [PATCH 1/3] Fix xml deserialization when xsi:nil="true" is set Add xml deserialization null check based on xsi:nil Expand visitor interface with a null data detect function Add default implementation for null detection in abstract visitor Add round-trip test for serializing ad de-serializing null object --- src/AbstractVisitor.php | 9 ++++++ src/GraphNavigator.php | 3 +- src/VisitorInterface.php | 10 +++++++ src/XmlDeserializationVisitor.php | 22 +++++++++++++++ tests/Fixtures/ObjectWithNullProperty.php | 14 ++++++++++ tests/Serializer/BaseSerializationTest.php | 32 ++++++++++++++++++++++ tests/Serializer/XmlSerializationTest.php | 11 ++++++++ 7 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/AbstractVisitor.php b/src/AbstractVisitor.php index 8fdbe0859..ec469c185 100644 --- a/src/AbstractVisitor.php +++ b/src/AbstractVisitor.php @@ -63,4 +63,13 @@ protected function getElementType($typeArray) } } + /** + * @param mixed $data + * + * @return bool + */ + public function isNullData($data) + { + return $data === null; + } } diff --git a/src/GraphNavigator.php b/src/GraphNavigator.php index c3846c305..bcccb60b6 100644 --- a/src/GraphNavigator.php +++ b/src/GraphNavigator.php @@ -119,9 +119,10 @@ public function accept($data, array $type = null, Context $context) $type = array('name' => $typeName, 'params' => array()); } + // If the data is null, we have to force the type to null regardless of the input in order to // guarantee correct handling of null values, and not have any internal auto-casting behavior. - else if ($context instanceof SerializationContext && null === $data) { + if ($visitor instanceof VisitorInterface && true === $visitor->isNullData($data)) { $type = array('name' => 'NULL', 'params' => array()); } diff --git a/src/VisitorInterface.php b/src/VisitorInterface.php index 7b8480047..0728975d8 100644 --- a/src/VisitorInterface.php +++ b/src/VisitorInterface.php @@ -138,4 +138,14 @@ public function getNavigator(); * @return object|array|scalar */ public function getResult(); + + /** + * Determine if the value evaluates to null. + * Used by the navigator to determine the correct data type. + * + * @param mixed $data + * + * @return bool + */ + public function isNullData($data); } diff --git a/src/XmlDeserializationVisitor.php b/src/XmlDeserializationVisitor.php index 8604b1008..904bc2322 100644 --- a/src/XmlDeserializationVisitor.php +++ b/src/XmlDeserializationVisitor.php @@ -387,4 +387,26 @@ private function getDomDocumentTypeEntitySubset($data) return $internalSubset; } + + /** + * Consider xml element value null if the xsi:nil attribute is set and therefore can't have a value + * @see https://www.w3.org/TR/xmlschema-1/#xsi_nil + * + * @param $data + * + * @return bool + */ + public function isNullData($data) + { + if ($data instanceof \SimpleXMLElement) { + $xsiAttributes = $data->attributes('http://www.w3.org/2001/XMLSchema-instance'); + + //We have to keep the isset quiet, some tests give error: `Node no longer exists`; even though it evaluates to false + if (@isset($xsiAttributes['nil']) && (string) $xsiAttributes['nil'] === 'true') { + return true; + } + } + + return $data === null; + } } diff --git a/tests/Fixtures/ObjectWithNullProperty.php b/tests/Fixtures/ObjectWithNullProperty.php index 63385988f..af88db15e 100644 --- a/tests/Fixtures/ObjectWithNullProperty.php +++ b/tests/Fixtures/ObjectWithNullProperty.php @@ -18,7 +18,21 @@ namespace JMS\Serializer\Tests\Fixtures; +use JMS\Serializer\Annotation\Type; + class ObjectWithNullProperty extends SimpleObject { + /** + * @var null + * @Type("string") + */ private $nullProperty = null; + + /** + * @return null + */ + public function getNullProperty() + { + return $this->nullProperty; + } } diff --git a/tests/Serializer/BaseSerializationTest.php b/tests/Serializer/BaseSerializationTest.php index 15a236f0e..96301c364 100644 --- a/tests/Serializer/BaseSerializationTest.php +++ b/tests/Serializer/BaseSerializationTest.php @@ -166,6 +166,25 @@ public function testSerializeNullObject() ); } + public function testDeserializeNullObject() + { + if (!$this->hasDeserializer()) { + $this->markTestSkipped(sprintf('No deserializer available for format `%s`', $this->getFormat())); + } + + $obj = new ObjectWithNullProperty('foo', 'bar'); + + /** @var ObjectWithNullProperty $dObj */ + $dObj = $this->serializer->deserialize( + $this->getContent('simple_object_nullable'), + ObjectWithNullProperty::class, + $this->getFormat() + ); + + $this->assertEquals($obj, $dObj); + $this->assertNull($dObj->getNullProperty()); + } + /** * @dataProvider getTypes */ @@ -1345,6 +1364,19 @@ public function testObjectWithNullableArrays() $this->assertEquals($this->getContent('nullable_arrays'), $this->serializer->serialize($object, $this->getFormat())); } + public function testIsNullDataWithNull() + { + /** @var VisitorInterface $visitor */ + foreach ($this->serializationVisitors as $visitor) { + $this->assertTrue($visitor->isNullData(null)); + } + + /** @var VisitorInterface $visitor */ + foreach ($this->deserializationVisitors as $visitor) { + $this->assertTrue($visitor->isNullData(null)); + } + } + abstract protected function getContent($key); abstract protected function getFormat(); diff --git a/tests/Serializer/XmlSerializationTest.php b/tests/Serializer/XmlSerializationTest.php index 54698b19b..98dd4dc9d 100644 --- a/tests/Serializer/XmlSerializationTest.php +++ b/tests/Serializer/XmlSerializationTest.php @@ -26,6 +26,7 @@ use JMS\Serializer\Handler\HandlerRegistry; use JMS\Serializer\Metadata\StaticPropertyMetadata; use JMS\Serializer\Naming\CamelCaseNamingStrategy; +use JMS\Serializer\Naming\PropertyNamingStrategyInterface; use JMS\Serializer\Naming\SerializedNameAnnotationStrategy; use JMS\Serializer\SerializationContext; use JMS\Serializer\Serializer; @@ -52,6 +53,7 @@ use JMS\Serializer\Tests\Fixtures\SimpleClassObject; use JMS\Serializer\Tests\Fixtures\SimpleObject; use JMS\Serializer\Tests\Fixtures\SimpleSubClassObject; +use JMS\Serializer\XmlDeserializationVisitor; use JMS\Serializer\XmlSerializationVisitor; use PhpCollection\Map; @@ -488,6 +490,15 @@ public function testDeserializeEmptyString() $this->deserialize('', 'stdClass'); } + public function testIsNullDataWithXSINilLabeledElement() + { + $namingStrategy = $this->getMockBuilder(PropertyNamingStrategyInterface::class)->getMock(); + $visitor = new XmlDeserializationVisitor($namingStrategy); + $element = simplexml_load_string(''); + + $this->assertTrue($visitor->isNullData($element)); + } + private function xpathFirstToString(\SimpleXMLElement $xml, $xpath) { $nodes = $xml->xpath($xpath); From 1f9021ecb8d536ba7d716faf69369d4fbe3c8a87 Mon Sep 17 00:00:00 2001 From: pbouwdewijn Date: Mon, 22 May 2017 10:50:32 +0200 Subject: [PATCH 2/3] Moved isNullData away to a separate `NullEvaluatorInterface` Only the XMLDeserializationVisitor implements the `NullEvaluatorInterface` Restored behaviour in the GraphNavigator for the SerializationContext (elseif branch) Removed now obsolete tests from the BaseSerializationTest --- src/AbstractVisitor.php | 10 ------- src/GraphNavigator.php | 12 ++++++-- src/NullEvaluatorInterface.php | 32 ++++++++++++++++++++++ src/VisitorInterface.php | 10 ------- src/XmlDeserializationVisitor.php | 15 ++++------ tests/Serializer/BaseSerializationTest.php | 13 --------- tests/Serializer/XmlSerializationTest.php | 5 ++-- 7 files changed, 51 insertions(+), 46 deletions(-) create mode 100644 src/NullEvaluatorInterface.php diff --git a/src/AbstractVisitor.php b/src/AbstractVisitor.php index ec469c185..ce18915e1 100644 --- a/src/AbstractVisitor.php +++ b/src/AbstractVisitor.php @@ -62,14 +62,4 @@ protected function getElementType($typeArray) return $typeArray['params'][0]; } } - - /** - * @param mixed $data - * - * @return bool - */ - public function isNullData($data) - { - return $data === null; - } } diff --git a/src/GraphNavigator.php b/src/GraphNavigator.php index bcccb60b6..c1b308d0b 100644 --- a/src/GraphNavigator.php +++ b/src/GraphNavigator.php @@ -119,10 +119,18 @@ public function accept($data, array $type = null, Context $context) $type = array('name' => $typeName, 'params' => array()); } - // If the data is null, we have to force the type to null regardless of the input in order to // guarantee correct handling of null values, and not have any internal auto-casting behavior. - if ($visitor instanceof VisitorInterface && true === $visitor->isNullData($data)) { + else if ($context instanceof SerializationContext && null === $data) { + $type = array('name' => 'NULL', 'params' => array()); + } + + // Sometimes data can convey null but is not of a null type. + // Visitors can have the power to add this custom null evaluation + if ($visitor instanceof VisitorInterface + && $visitor instanceof NullEvaluatorInterface + && $visitor->evaluatesToNull($data) === true + ) { $type = array('name' => 'NULL', 'params' => array()); } diff --git a/src/NullEvaluatorInterface.php b/src/NullEvaluatorInterface.php new file mode 100644 index 000000000..03646c137 --- /dev/null +++ b/src/NullEvaluatorInterface.php @@ -0,0 +1,32 @@ + + * + * 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; + +interface NullEvaluatorInterface +{ + /** + * Determine if a value conveys a null value. + * An example could be an xml element (Dom, SimpleXml, ...) that is tagged with a xsi:nil attribute + * + * @param mixed $value + * + * @return bool + */ + public function evaluatesToNull($value); +} diff --git a/src/VisitorInterface.php b/src/VisitorInterface.php index 0728975d8..7b8480047 100644 --- a/src/VisitorInterface.php +++ b/src/VisitorInterface.php @@ -138,14 +138,4 @@ public function getNavigator(); * @return object|array|scalar */ public function getResult(); - - /** - * Determine if the value evaluates to null. - * Used by the navigator to determine the correct data type. - * - * @param mixed $data - * - * @return bool - */ - public function isNullData($data); } diff --git a/src/XmlDeserializationVisitor.php b/src/XmlDeserializationVisitor.php index 904bc2322..bcf5a08b0 100644 --- a/src/XmlDeserializationVisitor.php +++ b/src/XmlDeserializationVisitor.php @@ -25,7 +25,7 @@ use JMS\Serializer\Metadata\ClassMetadata; use JMS\Serializer\Metadata\PropertyMetadata; -class XmlDeserializationVisitor extends AbstractVisitor +class XmlDeserializationVisitor extends AbstractVisitor implements NullEvaluatorInterface { private $objectStack; private $metadataStack; @@ -389,17 +389,14 @@ private function getDomDocumentTypeEntitySubset($data) } /** - * Consider xml element value null if the xsi:nil attribute is set and therefore can't have a value - * @see https://www.w3.org/TR/xmlschema-1/#xsi_nil - * - * @param $data + * @param mixed $value * * @return bool */ - public function isNullData($data) + public function evaluatesToNull($value) { - if ($data instanceof \SimpleXMLElement) { - $xsiAttributes = $data->attributes('http://www.w3.org/2001/XMLSchema-instance'); + if ($value instanceof \SimpleXMLElement) { + $xsiAttributes = $value->attributes('http://www.w3.org/2001/XMLSchema-instance'); //We have to keep the isset quiet, some tests give error: `Node no longer exists`; even though it evaluates to false if (@isset($xsiAttributes['nil']) && (string) $xsiAttributes['nil'] === 'true') { @@ -407,6 +404,6 @@ public function isNullData($data) } } - return $data === null; + return $value === null; } } diff --git a/tests/Serializer/BaseSerializationTest.php b/tests/Serializer/BaseSerializationTest.php index 96301c364..d0e7c71f6 100644 --- a/tests/Serializer/BaseSerializationTest.php +++ b/tests/Serializer/BaseSerializationTest.php @@ -1364,19 +1364,6 @@ public function testObjectWithNullableArrays() $this->assertEquals($this->getContent('nullable_arrays'), $this->serializer->serialize($object, $this->getFormat())); } - public function testIsNullDataWithNull() - { - /** @var VisitorInterface $visitor */ - foreach ($this->serializationVisitors as $visitor) { - $this->assertTrue($visitor->isNullData(null)); - } - - /** @var VisitorInterface $visitor */ - foreach ($this->deserializationVisitors as $visitor) { - $this->assertTrue($visitor->isNullData(null)); - } - } - abstract protected function getContent($key); abstract protected function getFormat(); diff --git a/tests/Serializer/XmlSerializationTest.php b/tests/Serializer/XmlSerializationTest.php index 98dd4dc9d..8d7183fed 100644 --- a/tests/Serializer/XmlSerializationTest.php +++ b/tests/Serializer/XmlSerializationTest.php @@ -490,13 +490,14 @@ public function testDeserializeEmptyString() $this->deserialize('', 'stdClass'); } - public function testIsNullDataWithXSINilLabeledElement() + public function testEvaluatesToNull() { $namingStrategy = $this->getMockBuilder(PropertyNamingStrategyInterface::class)->getMock(); $visitor = new XmlDeserializationVisitor($namingStrategy); $element = simplexml_load_string(''); - $this->assertTrue($visitor->isNullData($element)); + $this->assertTrue($visitor->evaluatesToNull($element)); + $this->assertTrue($visitor->evaluatesToNull(null)); } private function xpathFirstToString(\SimpleXMLElement $xml, $xpath) From f31243fc174fdb57060fdd21c170671bb76347c9 Mon Sep 17 00:00:00 2001 From: Asmir Mustafic Date: Fri, 23 Jun 2017 14:03:51 +0200 Subject: [PATCH 3/3] nul evaluator interface is a visitor now --- src/GraphNavigator.php | 6 +----- ...EvaluatorInterface.php => NullAwareVisitorInterface.php} | 4 ++-- src/XmlDeserializationVisitor.php | 4 ++-- tests/Serializer/XmlSerializationTest.php | 4 ++-- 4 files changed, 7 insertions(+), 11 deletions(-) rename src/{NullEvaluatorInterface.php => NullAwareVisitorInterface.php} (90%) diff --git a/src/GraphNavigator.php b/src/GraphNavigator.php index c1b308d0b..c3921f3b3 100644 --- a/src/GraphNavigator.php +++ b/src/GraphNavigator.php @@ -124,13 +124,9 @@ public function accept($data, array $type = null, Context $context) else if ($context instanceof SerializationContext && null === $data) { $type = array('name' => 'NULL', 'params' => array()); } - // Sometimes data can convey null but is not of a null type. // Visitors can have the power to add this custom null evaluation - if ($visitor instanceof VisitorInterface - && $visitor instanceof NullEvaluatorInterface - && $visitor->evaluatesToNull($data) === true - ) { + if ($visitor instanceof NullAwareVisitorInterface && $visitor->isNull($data) === true) { $type = array('name' => 'NULL', 'params' => array()); } diff --git a/src/NullEvaluatorInterface.php b/src/NullAwareVisitorInterface.php similarity index 90% rename from src/NullEvaluatorInterface.php rename to src/NullAwareVisitorInterface.php index 03646c137..619e0c5b3 100644 --- a/src/NullEvaluatorInterface.php +++ b/src/NullAwareVisitorInterface.php @@ -18,7 +18,7 @@ namespace JMS\Serializer; -interface NullEvaluatorInterface +interface NullAwareVisitorInterface extends VisitorInterface { /** * Determine if a value conveys a null value. @@ -28,5 +28,5 @@ interface NullEvaluatorInterface * * @return bool */ - public function evaluatesToNull($value); + public function isNull($value); } diff --git a/src/XmlDeserializationVisitor.php b/src/XmlDeserializationVisitor.php index bcf5a08b0..9089cdaba 100644 --- a/src/XmlDeserializationVisitor.php +++ b/src/XmlDeserializationVisitor.php @@ -25,7 +25,7 @@ use JMS\Serializer\Metadata\ClassMetadata; use JMS\Serializer\Metadata\PropertyMetadata; -class XmlDeserializationVisitor extends AbstractVisitor implements NullEvaluatorInterface +class XmlDeserializationVisitor extends AbstractVisitor implements NullAwareVisitorInterface { private $objectStack; private $metadataStack; @@ -393,7 +393,7 @@ private function getDomDocumentTypeEntitySubset($data) * * @return bool */ - public function evaluatesToNull($value) + public function isNull($value) { if ($value instanceof \SimpleXMLElement) { $xsiAttributes = $value->attributes('http://www.w3.org/2001/XMLSchema-instance'); diff --git a/tests/Serializer/XmlSerializationTest.php b/tests/Serializer/XmlSerializationTest.php index 8d7183fed..115af07e0 100644 --- a/tests/Serializer/XmlSerializationTest.php +++ b/tests/Serializer/XmlSerializationTest.php @@ -496,8 +496,8 @@ public function testEvaluatesToNull() $visitor = new XmlDeserializationVisitor($namingStrategy); $element = simplexml_load_string(''); - $this->assertTrue($visitor->evaluatesToNull($element)); - $this->assertTrue($visitor->evaluatesToNull(null)); + $this->assertTrue($visitor->isNull($element)); + $this->assertTrue($visitor->isNull(null)); } private function xpathFirstToString(\SimpleXMLElement $xml, $xpath)