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

Fixed an issue with import/export of multiple entities #5930

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

zyhfish
Copy link
Contributor

@zyhfish zyhfish commented Jan 20, 2024

Closes #5894

remove the child node with null reference check.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

I would like to discuss this in the next TAG meeting. This would fix the issue at hand but I am not sure this is what we should do or find another way for that obsolete serialization higher up.

@david-poindexter
Copy link
Contributor

Agreed. Thanks so much for submitting this PR @zyhfish 🎉 Our next meeting is on Jan 23.

@zyhfish
Copy link
Contributor Author

zyhfish commented Jan 29, 2024

Hi @valadas , another possible solution is handle the XmlSerializer.UnknownElement or UnknownNode event handler to add the obsoleted properties manually, but IMO this is not needed as we need to remove them finally in the result.

@valadas
Copy link
Contributor

valadas commented Jan 29, 2024

HI @zyhfish my concern is not just about this specific bug but overall serialization by others on these objects. Being public and providing a base class any 3rd party could potentially have their own permissions inherited by one of these classes to do something custom and #5841 caused a breaking change in that respect.

I took a stab at implementing IXmlSerializable and quickly discovered that it was a bad idea because then any subclass would call the highest inherited class implementation unless we override those methods in the whole chain of inheritance. So it would not serialize any lower details.

I ran out of time to dig further, so I'll have to revisit this but curious about what people think on #5943 which has a different approach to simply having both casings present and making the wrong case property [CLSCompliant(false)]

@valadas
Copy link
Contributor

valadas commented Jan 29, 2024

That being said I did not yet try UnkownElement or UnkownNode, not sure on that specific one but could be something as long as we don't have the same issue as in IXmlSerializable implementation

@bdukes bdukes added this to the 9.13.3 milestone Feb 1, 2024
@mitchelsellers mitchelsellers merged commit 58dacd0 into dnnsoftware:develop Feb 20, 2024
3 checks passed
@valadas valadas changed the title Fix #5894: remove the child node with null reference check. Fixed an issue with import/export of multiple entities Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot export modules
5 participants