search

facebookarchive / facebook-instant-articles-sdk-php

The Facebook Instant Articles SDK for PHP provides a native interface for creating and publishing Instant Articles.
https://instantarticles.fb.com/
Other
232 stars 148 forks source link

convert &{entitityname}; in output to &{entityname}; #275

Closed timjacobi closed 6 years ago

timjacobi commented 7 years ago

All classes extending Element make use of createTextNode which converts symbols to their respective HTML entities e.g. & -> &

The inputs are not sanitized and therefore if they contain entities the & part will be converted to & e.g. " -> "

This commit adds a test to prove this and fixes the problem by converting the render output back.

Fixes https://github.com/Automattic/facebook-instant-articles-wp/issues/693

everton-rosario commented 6 years ago

@timjacobi Nice fix! I have mixed feelings about this PR since it will conflict with #287. Into that particular PR I just change the properties src, link and href, which I was trying to be sure to be dealing with only URLs/string.

But then, this PR is replacing any & back to & in cases that maybe we shouldn't. Example, if we are transforming any & into & this case will break:

Paragraph::create()->appendText("This is how e can use & tags into normal paragraphs")->render();

What do you think @timjacobi? Do you think we should merge these 2 PRs into one? But then we can check for adding more test cases for rendering and also transforming. I see the transforming testCases more important in this case, in a sense that any parsed content, should be transformed correctly.

timjacobi commented 6 years ago

I don't think that the two PRs will conflict but yours is definitely more important. Since we've both been good citizen and added tests I suggest we merge your PR first and then see if my PR rebased onto yours breaks any tests.

pestevez commented 6 years ago

Hey, @timjacobi - can you please resolve the conflicts so we can take another look at the PR? Thanks!

vkama commented 6 years ago

hey guys, I am jumping on this late. So #287 was merged and Tim rebased recently and the tests are passing, this means we are good to merge or should I do a more in depth test?

timjacobi commented 6 years ago

It's good to go.