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

JSON Data tags broken #2182

Closed
Cosrnos opened this issue May 24, 2018 · 7 comments
Closed

JSON Data tags broken #2182

Cosrnos opened this issue May 24, 2018 · 7 comments
Assignees
Milestone

Comments

@Cosrnos
Copy link

Cosrnos commented May 24, 2018

It looks like you're relying on an old build of Automattic's AMP that is using XML instead of HTML for serialization. As a result, JSON config for the new amp-consent tag is breaking across AMP Pages for people trying to use tags that rely on this data.

Original bug: ampproject/amp-wp#755
Original fix: ampproject/amp-wp#891

cursor_and_hello_world____user_s_blog_

@Cosrnos Cosrnos changed the title JSON Sanitizing is too agressive JSON Data tags broken May 24, 2018
@ahmedkaludi ahmedkaludi added this to the W14 milestone Jun 6, 2018
@Sejiro-Hiko
Copy link
Contributor

@Cosrnos are you still facing this issue?

@Cosrnos
Copy link
Author

Cosrnos commented Jun 8, 2018

@Sejiro-Hiko Yep, we still are seeing a conflict with this behavior and the amp-geo/amp-consent tags

@Sejiro-Hiko
Copy link
Contributor

@Cosrnos can you please reach us out here https://ampforwp.com/support/ along with your site url, I'll look into the issue on your end.

@Cosrnos
Copy link
Author

Cosrnos commented Jun 11, 2018

@Sejiro-Hiko We don't have a site, we're developing a plugin and one of the behaviors is dropping an amp-consent and amp-geo tag onto the page, but anytime we try to do this via the sanitizer hook originally provided by the AMP plugin from Automattic that you've forked, your plugin tries to wrap the JSON config for those objects in CDATA. It looks like you haven't pulled in the changes from the AMP plugin since it was fixed in their 0.7.0 tag. I've already linked the bug and fix from that repo in my original issue.

You can test this yourself by adding a sanitizer and inserting an amp-geo tag into the page with some basic config.

@Sejiro-Hiko
Copy link
Contributor

@Cosrnos can you please test this developmental version https://github.com/ahmedkaludi/accelerated-mobile-pages/archive/0.9.96.zip as we've resolved the issue already and if it still appears then with this one it should be gone please test and let us know so that we can merge this in the master and release it.

@MohammedKaludi
Copy link
Collaborator

@Cosrnos Can you please check and let us know if it fixed, because we are going to push the update on Monday, if we did not get any response from you till Monday, then we have to close the ticket.

You can reopen it again. But will be assigned into next milestone.

@MohammedKaludi
Copy link
Collaborator

@Cosrnos Closing the ticket for now. Please reopen if the issue persists.

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

No branches or pull requests

4 participants