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

Replace invalid xml utf-8 chars in tag contents. #23

Merged
merged 2 commits into from
Sep 21, 2014
Merged

Conversation

trepmag
Copy link
Contributor

@trepmag trepmag commented Sep 17, 2014

I was getting issue to validate some feeds and finally found that not all utf-8 chars are valid for xml: http://www.phpwact.org/php/i18n/charsets#xml

So this commit insert the function found in the above link and use it when a string must be inserted into content tags.

@mibe mibe added the bug label Sep 17, 2014
@mibe
Copy link
Owner

mibe commented Sep 17, 2014

Yeah I can confirm this bug. This code reproduces it:

$TestFeed->setTitle("Test\x18Test");

The problem is that the commit works for usual content in tags, but not for attributes. This will not validate:

$TestFeed->setAtomLink('http://pubsubhubbub.appspot.com', "hub\x18");

I think it would be better to implement the filtering of the illegal chars in the makeNode() method instead.

@mibe mibe closed this Sep 17, 2014
@mibe
Copy link
Owner

mibe commented Sep 17, 2014

Oops, wrong button... 😣

…e values.

Feed::utf8_for_xml() method used in the Feed::makeNode() method.
@trepmag
Copy link
Contributor Author

trepmag commented Sep 21, 2014

Mibe, not sure you've been notified on the above commit...

mibe added a commit that referenced this pull request Sep 21, 2014
Replace invalid xml utf-8 chars in tag contents.
@mibe mibe merged commit 9fd75b1 into mibe:master Sep 21, 2014
@mibe
Copy link
Owner

mibe commented Sep 21, 2014

Yes, I didn't get a notify. Thanks for your work!

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

Successfully merging this pull request may close these issues.

None yet

2 participants