Skip to content

Sitemaps: Use proper encoding#7185

Merged
zinigor merged 8 commits intomasterfrom
kraftbj-patch-3
May 30, 2017
Merged

Sitemaps: Use proper encoding#7185
zinigor merged 8 commits intomasterfrom
kraftbj-patch-3

Conversation

@kraftbj
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj commented May 15, 2017

Fixes #6822

This borrows from the same approaches used by Core to prepare content for RSS feeds, which would pass XML validation.

@kraftbj kraftbj added [Feature] Sitemaps [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels May 15, 2017
@kraftbj kraftbj requested a review from nbloomf May 15, 2017 19:47
Copy link
Copy Markdown
Contributor

@nbloomf nbloomf left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I left a few minor comments. Other than that, I'm afraid the PR doesn't fix the issue on one of the sites where I thing I was experiencing the problem:
https://jeremy.hu/image-sitemap-1.xml

);

$title = esc_html( $post->post_title );
$title = apply_filters( 'the_title_rss', $post->post_title );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add something like /** This filter is already documented in wp-includes/feed.php */ to avoid tripping the parser?

}

$caption = esc_html( $post->post_excerpt );
$caption = apply_filters( 'the_excerpt_rss', $post->post_excerpt );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add the docblock here as well?


// Prepare the content like get_the_content_feed()
$content = $post->post_content;
$content = apply_filters( 'the_content', $content );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add the docblock here as well?

$content = $post->post_content;
$content = apply_filters( 'the_content', $content );
$content = str_replace(']]>', ']]>', $content);
$content = apply_filters( 'the_content_feed', $content, 'rss2' );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add the docblock here as well?

'lastmod' => jp_sitemap_datetime( $post->post_modified_gmt ),
'video:video' => array(
'video:title' => esc_html( $post->post_title ),
'video:title' => apply_filters( 'the_title_rss', $post->post_title ),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add the docblock here as well?

'news:language' => $language,
),
'news:title' => esc_html( $post->post_title ),
'news:title' => apply_filters( 'the_title_rss', $post->post_title ),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add the docblock here as well?

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels May 16, 2017
@kraftbj
Copy link
Copy Markdown
Contributor Author

kraftbj commented May 17, 2017

Updated inline docs per @jeherve and double-checking his testing after rebuilding the sitemap.

@kraftbj kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels May 17, 2017
@jeherve
Copy link
Copy Markdown
Member

jeherve commented May 18, 2017

Still no luck I'm afraid.

  1. I applied this branch.
  2. I deleted all the sitemaps from my database.
  3. I deactivated the sitemaps feature and activated it back.
  4. I waited a few minutes for the sitemaps to be built.
  5. The errors still seem to be there. https://jeremy.hu/image-sitemap-1.xml

@jeherve jeherve added this to the 5.0 milestone May 29, 2017
@eliorivero
Copy link
Copy Markdown
Contributor

It's funny because an ampersand is still not converted to an entity by esc_attr, esc_html or htmlentities2. I only got it working using htmlentities but it's not ideal since if there was something already converted to an entity it will do it again.

@eliorivero
Copy link
Copy Markdown
Contributor

Ok so the only way I found is to use htmlentities. As agreed with the others, I added a commit with it. Let's reverse it if you consider it risky.

Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

htmlentities fixes the problem for me. It's not perfect, but it works! 🚢

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Has Changelog and removed [Status] Needs Review This PR is ready for review. labels May 30, 2017
jeherve added a commit that referenced this pull request May 30, 2017
@zinigor zinigor merged commit ef4573c into master May 30, 2017
@zinigor zinigor deleted the kraftbj-patch-3 branch May 30, 2017 13:16
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 30, 2017
eliorivero pushed a commit that referenced this pull request May 30, 2017
* Changelog: first pass at a changelog for 5.0

* Changelog: delete 4.9 testing list.

* Changelog: update minimum WP version to match ver. in jetpack.php

Fixes #7158

* Changelog: add #6051

* Changelog: add #6753

* Changelog: add #6928

* Changelog: add #6964

* Changelog: add #7014

* Changelog: add #7057

* Changelog: add #7060

* Changelog: add #7068

* Changelog: add #7070

* Changelog: add #7072

* Changelog: add #7071

* Changelog: add release date and post shortlink.

* Changelog: add #7094

* Changelog: add #7100

* Changelog: add #7108

* Changelog: add #7113

* Changelog: add #7123

* Changelog: add #7135

* Changelog: add #7143

* Changelog: add #7151

* Changelog: add #6996

* Changelog: add #7105

* Changelog: add #7132

* Changelog: add #7166

* Changelog: fix typo in 4.9 changelog.

* Changelog: remove older releases' changelogs.

@see p1HpG7-42e-p2

* Changelog: add #7090

* Changelog: add #7095

* Changelog: add #7112

* Changelog: add #7115

* Changelog: add #7122

* Changelog: add #7137

* Changelog: add #7138

* Changelog: add #7140

* Changelog: add #7154

* Changelog: add ##7155

* Changelog: add #7163

* Changelog: add #7167

* Changelog: add #7171

* Changelog: add #7180

* Changelog: add #7181

* Changelog: add #7183

* Changelog: add #7184

* Changelog: add #7189

* Changelog: add #7191

* Changelog: add #7193

* Changelog: add #7198

* Changelog: add #7200

* Changelog: add #7209

* Changelog: add #7212

* Testing list: add instructions for #7115

* Changelog: add #7188

* Changelog: add #7205

* Changelog: add #7225

* Changelog: add #6872

* Changelog: add #7107

* Changelog: add #7118

* Changelog: add #7142

* Changelog: add #7170

* Changelog: add #7210

* Changelog: add #7218

* Changelog: add #7232

* Changelog: add #7211

* Changelog: add #7213

* Changelog: add #7229

* Changelog: add #7230

* Changelog: add #7214

* Draft changelog for 5.0

* Changelog updates: 2nd pass at a clearer changelog.

- Fix typos.
- Use consistent tense and tone across all changelog.
- Remove unclear items.

* Changelog: add #7026

* Changelog: add #7058

* Changelog: add #7125

* Changelog: add #7249

* Changelog: add #7185

* add mentions of image widget migration

* Changelog: add info about new output for CLI command.

* Changelog: add WP version number matching the new Image Widget.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants