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

Generate namespaced element on XmlList entries #301

Merged
merged 3 commits into from May 6, 2016

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Jul 16, 2014

Things done with this patch:

  • Merged XmlList and XmlMap Namespace support #237
  • Added documentation for XmlList and XmlMap Namespace support #237
  • @XmlNamespace(uri="http://example.com/namespace") acts as default namespace
  • XmlDeserializationVisitor
    • Removed a lot of XPath stuff in flavor of simpler SimpleXMLElement::children method
    • Added $objectMetadataStack to access class metadata from a property
  • XmlSerializationVisitor
    • Removed unnecessary element prefixing when the document/node default namespace match the desired namesapce
    • Removed some duplicated code, adding createElement and setAttributeOnNode methods

@goetas goetas changed the title Generate namespaced element on XmlList entries [WIP] Generate namespaced element on XmlList entries Jul 17, 2014
@goetas goetas changed the title [WIP] Generate namespaced element on XmlList entries Generate namespaced element on XmlList entries Jul 18, 2014
@goetas
Copy link
Collaborator Author

goetas commented Jul 18, 2014

@skler Are you still working on this?

@goetas
Copy link
Collaborator Author

goetas commented Jul 18, 2014

@schmittjoh Tests passes on my env (PHPUnit 3.7.28 and PHP 5.5.12)

} else {
$element = $this->document->createElement($elementName);
$classMetadata = $this->objectMetadataStack->top();
$defaultNamespace = isset($classMetadata->xmlNamespaces[''])?$classMetadata->xmlNamespaces['']:null;
Copy link
Owner

Choose a reason for hiding this comment

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

How about adding a method for this to the metadata class, we seem to repeat this in a couple of places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if an implementation as:

private function getClassDefaultNamespace(ClassMetadata $metadata)
{
    return (isset($metadata->xmlNamespaces[''])?$metadata->xmlNamespaces['']:null);
}

would be enough.

put $classMetadata = $this->objectMetadataStack->top(); inside is not possible...

@mfoti
Copy link

mfoti commented Jul 22, 2014

@goetas No, but if you need support I'm here

@goetas
Copy link
Collaborator Author

goetas commented Jul 23, 2014

News on this?

@goetas
Copy link
Collaborator Author

goetas commented Aug 1, 2014

Tests failures are related to #292 and #300

@RSully
Copy link

RSully commented Sep 26, 2014

What's holding this up?

@natebrunette
Copy link

+1

Is there a workaround for this in the meantime? My issue is that the node has a prefix and won't deserialize without specifying it.

@mfoti
Copy link

mfoti commented Dec 3, 2014

@schmittjoh could be integrated this? If you like other fixes tell us.

@xcompass
Copy link

+1

My current workaround is to remove the namespace prefix before sending it to deserialize.

@goetas
Copy link
Collaborator Author

goetas commented Dec 20, 2014

rebased some commits to have a clean commit list

@Tobur
Copy link

Tobur commented Jan 26, 2015

How it's going? I need this functionality 😢

@goetas
Copy link
Collaborator Author

goetas commented Jan 26, 2015

@Tobur you can try to use my fork (see here https://github.com/goetas/xsd2php how to use it into your composer.json)

@holtkamp
Copy link
Contributor

+1, reluctant to use a fork when a PR seems ready to go...

@RSully
Copy link

RSully commented Apr 3, 2015

Looks like this needs a rebase. Would love an update.

@hiend
Copy link

hiend commented Apr 7, 2015

+1

@maZahaca
Copy link

Hey, what's up here?

@discordier
Copy link
Contributor

I also want to leave my +1 on this one here as I just ran into exactly the same wall, array element has to be in a namespace.

@symm
Copy link

symm commented May 29, 2015

+1

2 similar comments
@coreequip
Copy link

+1

@PATROMO
Copy link

PATROMO commented Jun 15, 2015

+1

@yceruto
Copy link

yceruto commented Oct 6, 2015

+1, merge?

@netsensei
Copy link

+1 for a merge.

@goetas
Copy link
Collaborator Author

goetas commented Oct 26, 2015

@schmittjoh any chance to see this merged? i can rebase it again...

@brianmc
Copy link

brianmc commented Nov 10, 2015

+1 for a merge, please let us know if we can help in any way to get this in.

@timdev
Copy link

timdev commented Jan 1, 2016

+1 for a merge.

@slawomirkania
Copy link

+1 merge

@goetas
Copy link
Collaborator Author

goetas commented May 5, 2016

Rebased again.
@schmittjoh Please consider reopening this. There are many use cases for this feature and also many people that have expressed they interest in this

@goetas
Copy link
Collaborator Author

goetas commented May 5, 2016

Test failures are unrelated and currently on master https://travis-ci.org/schmittjoh/serializer/jobs/126569375

@@ -35,6 +35,7 @@ class PropertyMetadata extends BasePropertyMetadata
public $xmlCollection = false;
public $xmlCollectionInline = false;
public $xmlEntryName;
public $xmlEntryNamespace;
Copy link
Owner

Choose a reason for hiding this comment

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

We should add that to serialize / unserialize a bit further down, no?

@schmittjoh
Copy link
Owner

Sorry for the wait, looks good, can you check the comment above, so we can merge this?

@goetas
Copy link
Collaborator Author

goetas commented May 6, 2016

done

@schmittjoh
Copy link
Owner

We should probably have an exception if someone deserializes an old version. Could you check the number of elements in unserialize and either throw an exception or set some default for the new property?

@goetas
Copy link
Collaborator Author

goetas commented May 6, 2016

done

@schmittjoh schmittjoh merged commit 37cc390 into schmittjoh:master May 6, 2016
@schmittjoh
Copy link
Owner

Thanks!

@mfoti
Copy link

mfoti commented May 6, 2016

#237 can be closed, dunno why I'm not between the contributors :( but happy to see this merged!
Thanks you all guys :)

@goetas
Copy link
Collaborator Author

goetas commented May 6, 2016

@skler my bad, after almost two years of pending PR yesterday i have rebased it and for some issues with my git repo i have squashed all previous commits into one... sorry, my bad

@holtkamp
Copy link
Contributor

holtkamp commented May 8, 2016

@schmittjoh would this justify a new minor release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet