Adds XML namespaces support

[ajgarlag]

I've tried to implement the XML namespaces support for serialization and deserialization.

Please, review it if you can and tell me what you think about.

[henrikbjorn]

Could use this. 👍

[ajgarlag]

I think this could be improved. If the XML namespaces are registered when the root simplexml element is created, we could unserialize the xml regadless the prefix used at the payload data. But the prepare function at https://github.com/ajgarlag/serializer/blob/xml-namespaces/src/JMS/Serializer/XmlDeserializationVisitor.php#L57 has no access to the ClassMetadata in order to call SimpleXMLElement::registerXPathNamespace with the namespaces declared at the XmlNamespace annotations

Any idea?

[schmittjoh]

It will be tricky to add the namespaces in the prepare method. However, I think it should also work if we add them when we start visiting an object, i.e. startVisitingObject, no?

[ajgarlag]

Yes, it should work, but I don't see how to access to the SimpleXMLElement inside XmlDeserializationVisitor::startVisitingObject in order to register the xpath namespaces

[schmittjoh]

In general this looks quite good.

I think we need some additional tests to ensure that the aliases which are used on deserialization do not have to match the aliases that were used on serialization as long as the namespace they refer to is the same.

Also, if you switch the @XmlPrefix to an attribute, you'll probably have to introduce a @XmlElement instead which we currently assume implicitly if nothing else is given.

[ajgarlag]

The problem I see with @XmlElement and @XmlAttribute, is that we have to detect if a property has both annotations and throw an exception. With my approach, there will be no conflict annotating a property as element and attribute.

Anyway, if you prefer to have @XmlElement and @XmlAttribute annotations with an option, I would name it prefix or better nsprefix to prevent any confusion.

[schmittjoh]

I'd prefer the @XmlElement approach as it is the same in Java, and we have mostly modeled the @Xml??? annotations around that.

I've also thought a bit more about the namespace resolution process. So far, we have mostly viewed a single class and then it makes sense to use only the prefix. However, let's assume you have a base class and annotate some namespace. Then, in the sub-class you again annotate some namespace, but re-define prefixes of the parent class with a different namespace.

/** @XmlNamespace("http://foo", prefix = "foo") */
abstract class BaseClass {
   /** @XmlAttribute(nsprefix = "foo") */ // namespace: http://foo
   private $bar;
}

/**
 * @XmlNamespace("http://foo", prefix = "old_foo")
 * @XmlNamespace("http://better.foo", prefix = "foo")
 */
class SubClass extends BaseClass {
   /** @XmlAttribute(nsprefix = "foo") */ // namespace: http://better.foo
   private $moo;
}

In order to support this, I think we need to resolve the nsprefix to the full URI when we store the metadata (instead of storing the prefix), and then again look up the prefix depending on the actual class during serialization. If you agree, maybe you can also add a test case for the above scenario?

[schmittjoh]

Another test-case that we need to define:

/** @XmlNamespace("http://foo") */
abstract class BaseClass { 
   /** @XmlAttribute */
   private $bar;
}

/** @XmlNamespace("http://better.foo") */
class SubClass extends BaseClass {
    /** @XmlAttribute */
    private $moo;
}

In such a case, some automatic resolution/generation of prefixes seems to be ideal to me. After all, the consuming code should never rely on the namespace prefix, but only ever of the full namespace URI; so the exact value of the prefix should be irrelevant except for cosmetic purposes/humans.

[ajgarlag]

@schmittjoh Finally here is the refactorization of @XmlPrefix in favor of @XmlAttribute and @XmlElement. After thinking about the problems with the same prefix referring to different namespaces, I've opted to require the full namespace uri.

Example:

    /**
     * @Type("string")
     * @Groups({"comments","post"})
     * @XmlElement(namespace="http://purl.org/dc/elements/1.1/");
     */
    private $title;

    /**
     * @Type("string")
     * @XmlAttribute(namespace="http://schemas.google.com/g/2005")
     * @Groups({"post"})
     */
    private $etag;

Please, review it if you can.

[schmittjoh]

Looks really good! Thanks for your work on this.

It's quite a big feature, and I think we still need a few more test cases; specifically for the following scenarios:

• No @XmlNamespace annotation defined for a namespace defined through @XmlAttribute/@XmlElement
• A test involving class inheritance + namespaces (see the examples above)
• Namespaces defined only for some properties, but not for others

[ajgarlag]

@schmittjoh

• First scenario: In JMS\Serializer\Tests\Fixtures\ObjectWithXmlNamespaces you can see the properties title and language annotated with a namespace http://purl.org/dc/elements/1.1/ not defined in @XmlNamespace
• Third scenario: In JMS\Serializer\Tests\Fixtures\BlogPost you can see some properties with namespace, like etag and some properties without namespace, like createdAt

Regarding the second scenario: are the child classes supposed to inherit the parent metadata?

If I try something like this:

/** @XmlNamespace("http://foo", prefix = "foo") */
abstract class BaseClass {
   /** @XmlAttribute(namespace = "http://foo") */
   public $bar;
}

/**
 * @XmlNamespace("http://foo", prefix = "old_foo")
 * @XmlNamespace("http://better.foo", prefix = "foo")
 */
class SubClass extends BaseClass {
   /** @XmlAttribute(namespace = "http://better.foo") */
   public $moo;
}

The ClassMetadata for SubClass has no property called bar, only one property (moo) is defined.

Am I doing something wrong?

[ajgarlag]

@schmittjoh

Forget my last comment about the second scenario. Finally, I've been able to test the @XmlNamespace annotation inheritance.

[ajgarlag]

@schmittjoh

Please review the new tests, specifically JMS\Serializer\Tests\Metadata\Driver\BaseDriverTest::testXmlNamespaceInheritanceMetadata https://github.com/ajgarlag/serializer/commit/687f8aeccb6f12c57a3d967460184bf128e4fa60#L2R207

I don't know if that is the expected behaviour with classes and subclasses

[ajgarlag]

ping @schmittjoh

[pborreli]

need this functionality 👍

[brianium]

How would this work with something like the XmlList attribute?

[holtkamp]

Works nice, +1 to merge into master

[marcospassos]

@schmittjoh can you merge this feature? We really need this and it's been 6 months...

[marcospassos]

@ajgarlag it should be always checked in relation of current node (the lookup algorithm checks from the current node up to the root), otherwise object's properties inside objects will have a auto-generated prefix, even if it was been declared:

Consider the following mapping:

<?xml version="1.0" encoding="UTF-8"?>
<serializer>
    <class name="Acme\Parent" xml-root-name="package">
        <xml-namespace uri="http://acme-parent.com"/>
        <property name="child" type="Acme\Child" />
    </class>
</serializer>

<?xml version="1.0" encoding="UTF-8"?>
<serializer>
    <class name="Acme\Child" xml-root-name="child">
        <xml-namespace uri="http://acme-child.com" prefix="dc" />
        <property name="data" type="string" xml-namespace="http://acme-child.com" />
    </class>
</serializer>

This is the current result:

<?xml version="1.0" encoding="UTF-8"?>
<parent xmlns="http://acme-parent.com">
  <child xmlns:ns-04de78c1="http://acme-child.com">
    <ns-04de78c1:data xmlns:ns-04de78c1="http://acme-child.com"><![CDATA[lorem-ipsum]]></ns-04de78c1:data>
  </child>
</parent>

This is the result doing the lookup in the current node and moving the call to addNamespaceAttributes in the method startVisitingObject out of the if statement:

<?xml version="1.0" encoding="UTF-8"?>
<parent xmlns="http://acme-parent.com">
  <child xmlns:dc="http://acme-child.com">
    <dc:data><![CDATA[lorem-ipsum]]></dc:data>
  </child>
</parent>

[jeserkin]

Could anyone say, what is missing to reach consensus on this topic? I recently came across an XML, that uses namespace and I need to deserialize it. Will help to resolve this issue in anyway I can if it is needed.

Anyone?

[MadMind]

Any updates on this issue?

[ajgarlag]

I am very busy these days, but I'll try to find time to work on this PR soon.

[ElectricMaxxx]

Hi maybe you can wait some more days. I am ready with changing the deserialization visitor to the same system the serialization visitor works: on the DOMDocument Objekt. All Tests (Even more than before) are Green for Deserialization of XML.

Only need to fix my history before i can make a PR. Maybe i will do it this night.

With this you will have a better basement for the namespace issue.

[jeserkin]

Any movement with that at all? Any current workaround for namespaces with what is already implemented?

[schmittjoh]

As far as I know there were some outstanding things, that @ajgarlag is working on, but maybe you can lend him a hand.

[ajgarlag]

@marcospassos Yesterday, @jeserkin contacted me to help with this PR and he is already working on it. Maybe you can contact him to avoid duplicate work.

I won't have any time to work on this PR until february, so I'll appreciate your help.

[marcospassos]

@ajgarlag thanks for the alert. @jeserkin, have you read my considerations above? BTW, these are the required changes to fix the mentioned bugs:
https://github.com/marcospassos/serializer/commit/99514ab96b0ac4d380c52888f7670749d9bb4650

[jeserkin]

@marcospassos read it recently. If you fixed needed bugs, then be sure to send PR to @ajgarlag or straight here as you want.

[marcospassos]

I'm not able to send a PR to @ajgard once I've rebased my fork. You can check the changes here (just two lines).

[jeserkin]

@marcospassos yes yes. I've seen them. I will send it then.

UPD: Is everyone getting nasty Segmentation fault (core dumped) during tests or is it only me?

UPD:* Hapens during TypeParserTest::testParamTypeMustEndWithBracket()

[ajgarlag]

I've found some time to rebase my old PR.

It now includes a test to expose the problem detected by @marcospassos and his fix.

It's located in a new branch in my repo called xml-namespaces-rebased.

Could you do a quick test of it?

[marcospassos]

thanks, @ajgarlag! Nice work!

[jeserkin]

Seems, that all tests are correct at xml-namespaces-rebased branch. Will it be merged into main stream now, @schmittjoh?

[schmittjoh]

Could you send a new PR for that, or update this PR?

[ajgarlag]

@schmittjoh I've updated the PR. Could you review it?

[schmittjoh]

Great work, thanks!

One thing which was not included in this PR is some documentation. Maybe someone can write a few lines about the new annotations/config options.

[jeserkin]

@schmittjoh, is this website based on docs directory in this repo?

[schmittjoh]

Yes.

[holtkamp]

Would this merge, and thereby the addition of important functionality for XML serialization, justify a new tagged version (for instance 0.15.0)? This way referencing to a stable version becomes easy with Composer ;)

[ajgarlag]

@holtkamp #219 should be fixed before tagging a new version.

[marcospassos]

@schmittjoh once #219 is merged, can we have a new tagged version?