Skip to content

Decode non-XML entities for translations#4056

Merged
akirk merged 1 commit intomasterfrom
fix/illegal-xml-entities
Jun 8, 2016
Merged

Decode non-XML entities for translations#4056
akirk merged 1 commit intomasterfrom
fix/illegal-xml-entities

Conversation

@akirk
Copy link
Copy Markdown
Member

@akirk akirk commented Jun 2, 2016 •
edited
Loading

Fixes #3857

Changes proposed in this Pull Request:

This replaces named HTML entities, for example ’ as it is used in French translations, so that it doesn't break the XSLT.

To test:

  • set the language to French
  • delete the jetpack_sitemap_xsl transient
  • access /sitemap.xsl and check that no named entity appears there

@akirk akirk added [Status] Needs Review This PR is ready for review. [Feature] Sitemaps labels Jun 2, 2016
@akirk akirk added this to the 4.1 milestone Jun 2, 2016
@jeherve jeherve modified the milestones: 4.0.4, 4.1 Jun 2, 2016
kraftbj
kraftbj reviewed Jun 2, 2016
View reviewed changes
modules/sitemaps/sitemap-xsl.php Outdated
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj Jun 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using ent2ncr instead? It's what Core uses for RSS feeds to get valid XML. https://developer.wordpress.org/reference/functions/ent2ncr/

Copy link
Copy Markdown
Member Author

@akirk akirk Jun 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, didn't know about it :)

@kraftbj kraftbj added the Bug When a feature is broken and / or not performing as intended label Jun 2, 2016
@akirk akirk force-pushed the fix/illegal-xml-entities branch 2 times, most recently from 1bbd01a to bd78a37 Compare June 2, 2016 17:07
@tobiasehlert
Copy link
Copy Markdown

tobiasehlert commented Jun 7, 2016

It looks to me that there is also an issue with the translation on lines 93 in Swedish.
Ändra frekvens should be Ändra frekvens instead I guess.

This needs to be fixed somehow
line 93 <th>' . esc_html__( 'Change Frequency', 'jetpack' ) . '</th>

@akirk
Convert named entities to numeric ones to fix invalid XML
462bda6 
Translations can contain named entities. props @kraftbj
@akirk akirk force-pushed the fix/illegal-xml-entities branch from bd78a37 to 462bda6 Compare June 7, 2016 07:23
@akirk
Copy link
Copy Markdown
Member Author

akirk commented Jun 7, 2016

@tobiasehlert Thanks for noticing, I have now covered all translated text with ent2ncr().

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jun 8, 2016

This seems to work well in my tests. LGTM!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jun 8, 2016
@akirk akirk merged commit 1a29d3e into master Jun 8, 2016
@akirk akirk deleted the fix/illegal-xml-entities branch June 8, 2016 15:45
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 8, 2016
@nbloomf nbloomf mentioned this pull request Dec 14, 2016
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Sitemaps

Projects

None yet

Milestone

4.0.4

Development

Successfully merging this pull request may close these issues.

5 participants

@akirk @tobiasehlert @jeherve @kraftbj @matticbot