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

Handle objects that overload stringification #7

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@davorg

davorg commented Dec 7, 2017

This fixes a problem that we've just seen at my current client. But I think it might be more widely useful.

And, yes, I know that the real fix is to stop using XML::Simple. But that's not going to happen any time soon :-(

@grantm

This comment has been minimized.

Show comment
Hide comment
@grantm

grantm Mar 17, 2018

Owner

Hi Dave

I wasn't ignoring your PR, I just have to work myself into the right frame of mind before diving back into the internals of XML::Simple. It's not a quick process.

Did you end up deploying code that relied on the change in your pull request? Or did it turn out to be a terrible idea for other reasons, causing you to give up and abandon it? If you ended up using it, then I'm happy to merge the PR. If you didn't, then I'd just as soon close it and pretend this never happened.

Your PR didn't include a test, so I wrote one and in the process realised that your change won't work for all objects that override stringification, but only those objects that are not built on blessed hashrefs or blessed arrayrefs. Apparently they always get serialised as if they were just plain hashrefs and arrayrefs (including any secret internal keys and values). I had forgotten that XMLout() worked that way and frankly I can't believe I ever thought it was a good idea. But the fact is it does work that way and has for over 15 years so it's safe to assume someone is relying on it..

I'm now trying to work out what a change to the docs would look like. If I mention that XMLout() will no longer die on an object that overrides stringification then some people are going to be surprised that the stringification overrides in their classes are not being called.

Thoughts?

Owner

grantm commented Mar 17, 2018

Hi Dave

I wasn't ignoring your PR, I just have to work myself into the right frame of mind before diving back into the internals of XML::Simple. It's not a quick process.

Did you end up deploying code that relied on the change in your pull request? Or did it turn out to be a terrible idea for other reasons, causing you to give up and abandon it? If you ended up using it, then I'm happy to merge the PR. If you didn't, then I'd just as soon close it and pretend this never happened.

Your PR didn't include a test, so I wrote one and in the process realised that your change won't work for all objects that override stringification, but only those objects that are not built on blessed hashrefs or blessed arrayrefs. Apparently they always get serialised as if they were just plain hashrefs and arrayrefs (including any secret internal keys and values). I had forgotten that XMLout() worked that way and frankly I can't believe I ever thought it was a good idea. But the fact is it does work that way and has for over 15 years so it's safe to assume someone is relying on it..

I'm now trying to work out what a change to the docs would look like. If I mention that XMLout() will no longer die on an object that overrides stringification then some people are going to be surprised that the stringification overrides in their classes are not being called.

Thoughts?

@davorg

This comment has been minimized.

Show comment
Hide comment
@davorg

davorg Mar 17, 2018

Hi Grant,
This change never made it into production, so I'm happy to just ignore the whole thing. Next time we're working in this area, I'm going to make a bigger push for just moving away from XML::Simple.
Thanks for looking into it though. Sorry it turned out to be a waste of time.
Dave...

davorg commented Mar 17, 2018

Hi Grant,
This change never made it into production, so I'm happy to just ignore the whole thing. Next time we're working in this area, I'm going to make a bigger push for just moving away from XML::Simple.
Thanks for looking into it though. Sorry it turned out to be a waste of time.
Dave...

@davorg davorg closed this Mar 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment