XML Serialization: fix #1601

[NikoGrano]

Q	A
Bug fix?	no (/yes)
New feature?	yes
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1601
License	MIT

This PR introduces a new capability to the XML serialization process, allowing properties annotated with #[XmlElement] to define attributes on sibling XML elements using the #[SerializedName("ElementName/@AttributeName")] syntax.

Problem:

Previously, when a property was configured with #[XmlElement] and #[SerializedName("ElementName/@AttributeName")], the serializer would attempt to create an XML element literally named ElementName/@AttributeName. This would lead to a DOMException due to invalid characters in the element name. This behavior was inconsistent with the deserialization process, which correctly interprets this syntax to map an XML attribute to a PHP property. This issue is described in #1601.

Solution:

The XmlSerializationVisitor::visitProperty method has updated: (simplified/tl;dr)

1. It now checks if a property's serializedName contains the /@ pattern
2. Conditions are met -> invoke new method trySerializePropertyAsAttributeOnSiblingElement.
3. New method parses the target element name and attribute from serializedName.
4. Try to handle namespaces and target element correctly 😅
5. If the property's value is null, the attribute is not rendered.
6. Implemented two new tests to serialize and assert to expected xml output. (As bonus, trying to deserialize generated xml for consistency, but there is something weird about it when using namespaces, which is not related to this issue)

Hopefully, this can be checked quickly, so I know how I continue with my own project using this.

[NikoGrano]

Seems like Scrutinizer is somehow broken.

Could somebody restart it or just ignore it...?

[scyzoryck]

Thanks for pull request. Looks like an useful feature!
Please ignore Strutinizer check :)

I left one small comment. Can you also add some documentation for it, please?
Do you plan to implement deserialization as well? :)

Best, Marcin.

[NikoGrano]

Thanks for the review!

The deserialization side of this feature already exists, as the serializer passes the name as an xpath query. I wasn't aware of this until Mirgen pointed it out in this answer.

However, there is an issue with deserialization when namespaces are involved. Using this example:

serializer/tests/Fixtures/ObjectWithElementAttributeSyntax.php

Lines 44 to 49 in 3ecd094

	/**
	* @Serializer\SerializedName("CustomElement")
	* @Serializer\Type("string")
	* @Serializer\XmlElement(cdata=false, namespace="http://example.com/ns1")
	*/
	public $customElementValue;

With the current PR, we can serialize $customElementValue successfully, but deserializing it results in a null/uninitialized value (depending on typed properties). This only happens when a namespace is present on #[XmlElement(namespace: ...)] - it works fine without namespaces.

In the tests, we first serialize the entity to XML, compare it to the expected result, then try to deserialize back into an object. We had to add this bypass to set customElementType to null, otherwise tests would fail:

serializer/tests/Serializer/XmlSerializationTest.php

Lines 662 to 664 in 3ecd094

	$object->customElementType = null;
	$deserializedObject = $this->deserialize($actualXml, ObjectWithElementAttributeSyntax::class);
	self::assertEquals($object, $deserializedObject);

I'd like to address this deserialization issue separately to keep this PR focused on the serialization side. In my use case, I don't need namespaces, so I'm comfortable with the current implementation. This is an edge case that likely won't affect 99.9% of users, but it would be good to document it or open an issue about it to track the namespace deserialization problem and possible interest fixing it.

For documentation, I'll add it as requested. Since this extends an "accidental feature" that was already working for deserialization, this PR completes the picture by providing serialization support. Do you have specific recommendations for how you'd like the documentation structured? I don't have any clue how to document this.

I'll implement the requested argument updates soonish after I hear back from you.

[scyzoryck]

Sounds like a plan! Sounds like we will be able to release it next week :)

[NikoGrano]

I continued working on this and got a bit carried away implementing the deserialization side as well, since I could see potential issues arising from incomplete support later.

I've fixed the deserialization issues with namespaces and added implementation to serialize attributes into the current object, essentially adding serialization support to this answer.

While testing the @ and @/ style names, I encountered several deserialization issues when using namespaces. I've addressed these in commit e1334734.

TLDR - After the latest changes, this PR now has full support for:

1. @AttributeName syntax: Serializes/deserializes a property as an attribute on the current XML element
2. ElementName/@AttributeName syntax: Serializes/deserializes a property as an attribute on a sibling XML element named ElementName

I'll work on documenting this feature now.

[NikoGrano]

There was error in tests when running on PHP 7.4:

1) JMS\Serializer\Tests\Serializer\XmlSerializationTest::testSerializeElementAttributeSyntax
TypeError: Argument 1 passed to JMS\Serializer\XmlSerializationVisitor::processValueForXmlAttribute() must be an instance of JMS\Serializer\mixed, string given, called in /home/runner/work/serializer/serializer/src/XmlSerializationVisitor.php on line 413

I removed the mixed type in commit 42fcf87.

FYI, totally not related to this PR, I can see this type being used also in UnionHandler::serializeUnion. This might cause some problems for ppl using 7.4.

serializer/src/Handler/UnionHandler.php

Lines 53 to 58 in 3c2d00a

	public function serializeUnion(
	SerializationVisitorInterface $visitor,
	mixed $data,
	array $type,
	SerializationContext $context
	): mixed {

Cheers!