Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Introduce transformString() function to encapsulate the transformation process in multibyte environment. #161

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

yssk22
Copy link
Contributor

@yssk22 yssk22 commented Aug 19, 2016

This PR

  • introduce a new transform function that supports multibyte encoding environment.

Current transform function requires DOMDocument which requires developers to support multibyte encoding on their side. There are several ways to do that and this issue is one of the common issues in PHP environment.

The new function gets rid of this effort from developers and they can just pass HTML string to get the transformed HTML.

…ransformation.

The function is for multibyte users not to be confused on how to prepare DOMDocument
in their environment.

For the case of mbstring is missing, we still keep processing by using meta tags
as a fallback, which may or may not work. The fallback can be found by setting the
log level as debug.

By using HTML entities, a geotag script source may contain HTML entities so it
should be imported as a CDATA node instead of a Text node.
@ghost ghost added the CLA Signed label Aug 19, 2016
@everton-rosario
Copy link
Contributor

everton-rosario commented Aug 19, 2016

@yssk22 This looks great!

Can you add a test case that uses CDATA escaping content for XHTML compatibility?

Like this:

      <figure class="op-interactive">
         <iframe src="http://example.com/custom-interactive.xhtml" class="column-width" height="60">
           <h1>カスタムコード</h1>
           <script>
               <![CDATA[alert('テスト');]]>
           </script>
         </iframe>
         <figcaption>このグラフィックは素晴らしい。</figcaption>
       </figure>

Just to be sure we won't have any problem with XHTML and double encoding of CDATA.

The PR looks awesome BTW.

@ghost ghost added the CLA Signed label Aug 19, 2016
@yssk22
Copy link
Contributor Author

yssk22 commented Aug 23, 2016

@everton-rosario thanks for reviewing PR. yes, XHTML compatibility is something we need to consider.

Currently, loadHTML (DOMDocument) does not do anything on parsing cdata and the script content can include CDATA tags. This causes an issue on Elements\GeoTag->isValid() and the code omit script in CDATA even we use the original transform function.

One of the workaround for developers is to write

<script type="text/javascript">
// <![CDATA[
script here
// ]]>
</script>

And this is a typical solution for browsers who cannot understand XHTML.

On the other hand, we can remove '' string from the original content. do you want to do this on SDK in this PR?

@ghost ghost added the CLA Signed label Aug 23, 2016
@everton-rosario
Copy link
Contributor

Thanks for raising it @yssk22.
Lets do it in another PR.
Filed one issue with that: #163

@ghost ghost added the CLA Signed label Aug 23, 2016
@everton-rosario
Copy link
Contributor

Shipping this.

🚢

@everton-rosario everton-rosario merged commit bde5292 into facebookarchive:master Aug 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants