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

see #732 Encoding the extra fields so it can be stored in the XML #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ernestob
Copy link
Member

@ernestob ernestob commented May 7, 2021

Hi @peterkeung and @pkamps,

Can you review this?
This is fixing the issue #732.

@pkamps
Copy link
Member

pkamps commented May 10, 2021

Hi Ernesto,

could you use 'CDATA' instead of the json encoding?

I believe it's better to stick with a single format (xml) and not use a combination of 2 formats XML and JSON.

@ernestob
Copy link
Member Author

Hi @pkamps, the issue is that $dom->createTextNode() is expecting a string as parameter; the value of $content['extra_fields'] is an array.

To solve this issue, we need to create the whole array structure in XML or serialize the array information, which is what I am doing here.

@pkamps
Copy link
Member

pkamps commented May 12, 2021

Ah ok, that makes it more complicated.

If it's not too complex, we could consider to make sure $content['extra_fields'] is always an array. Then turn that array into XML. But if that's very complex the json en/decode method is OK to me.

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.

2 participants