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

Fix jsonify name in seo.html application/ld+json script #1485

Merged
merged 1 commit into from Jan 17, 2018

Conversation

Projects
None yet
2 participants
@d00616
Contributor

d00616 commented Jan 17, 2018

When the name includes " then the resulting schema is invalid. This is a fix.

@mmistakes

This comment has been minimized.

Owner

mmistakes commented Jan 17, 2018

Thanks for the PR.

Question, would the jsonify filter be a better option than xml_escape, or does that not properly escape double quotes?

@d00616

This comment has been minimized.

Contributor

d00616 commented Jan 17, 2018

To reproduce, set the "name:" field to a string "This is a "Test"" and validate the result via https://search.google.com/structured-data/testing-tool

Without this fix seo.html produces something like:

<script type="application/ld+json">
    {
      "@context" : "http://schema.org",
      "@type" : "Organization",
      "name" : "This is a "Test"",
      "url" : "http://1.2.3.4:4000",
      "sameAs" : null
    }
  </script>
@d00616

This comment has been minimized.

Contributor

d00616 commented Jan 17, 2018

jsonify is the better option.

@mmistakes

This comment has been minimized.

Owner

mmistakes commented Jan 17, 2018

@d00616 You have to remove the quotes around the variable. To keep things consistent I'd prefer to use jsonify like the others.

This would be the change:

<script type="application/ld+json">
{
  "@context": "http://schema.org",
   "@type": "{% if site.social.type %}{{ site.social.type }}{% else %}Person{% endif %}",
   "name": {{ site.social.name | default: site.name | jsonify }},
   "url": {{ seo_url | jsonify }},
   "sameAs": {{ site.social.links | jsonify }}
}
</script>

Which produces this valid JSON:

<script type="application/ld+json">
{
  "@context": "http://schema.org",
  "@type": "Person",
  "name": "This is a \"Test\"",
  "url": null,
  "sameAs": null
}
</script>

@d00616 d00616 changed the title from Fix: escape name in seo.html application/ld+json script to Fix jsonify name in seo.html application/ld+json script Jan 17, 2018

@d00616

This comment has been minimized.

Contributor

d00616 commented Jan 17, 2018

It's changed.

@mmistakes mmistakes merged commit 6fee80f into mmistakes:master Jan 17, 2018

@d00616

This comment has been minimized.

Contributor

d00616 commented Jan 17, 2018

Thank you.

@d00616 d00616 deleted the d00616:fix_ld_json branch Jan 17, 2018

martinbjeldbak added a commit to martinbjeldbak/martinbjeldbak.github.io that referenced this pull request Feb 1, 2018

Merge remote-tracking branch 'upstream/master'
* upstream/master: (528 commits)
  Fix docs. Change Isaac Newton with Albert Einstein (mmistakes#1508)
  Replace `|` with HTML entity when used as title separator
  Update stale.yml
  Remove ignore file
  Fix border bottom for Gist line numbers
  Update CHANGELOG and history
  fix a typo in 04-upgrading.md (mmistakes#1487)
  Update CHANGELOG and history
  Remove extra spaces after :
  Fix jsonify name in seo.html application/ld+json script (mmistakes#1485)
  Update CHANGELOG and history
  Add archive feature row test page
  Add archive feature row test page
  Adjust feature row styling when used on an `archive` layout
  Remove misleading underline hover state
  Replace toc include with `toc: true`
  Remove base_path include from pages
  Reduce font-size of page meta in list/grid items
  Underline archive item titles
  Update issue template
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment