RavanH / xml-sitemap-feed

XML Sitemap & Google News feeds
GNU General Public License v2.0
16 stars 21 forks source link

Add hooks for XML namespaces #35

Closed khromov closed 5 years ago

khromov commented 5 years ago

I am working on a plugin that adds multilingual sitemap links to the sitemaps generated by this plugin.

When doing this, you need to add an XML namespace to generate valid XML, but there is no hook for this. I have added hooks that are required for this to work properly.

Here is an example of the hook in use. This is using Polylang for multilingual support to add the xhtml namespace and print the alternative languages the post is available in to the sitemap.

// This is an example of one of the new hooks!
add_action('xmlsf_urlset', function ($type) {
    if($type === 'post_type') {
    echo 'xmlns:xhtml="http://www.w3.org/1999/xhtml"' . PHP_EOL;
    }
});

// This hooks already exists, thanks!
add_action('xmlsf_tags_after', function () { // xmlsf_tags_after
  $languages = pll_languages_list();
  foreach ($languages as $language) {
    $id              = get_the_ID();
    $translated_post = pll_get_post($id, $language);

    if ($translated_post && $translated_post !== $id) {
      ?>
    <xhtml:link rel="alternate" hreflang="<?php echo $language; ?>" href="<?php echo get_the_permalink(pll_get_post($id, $language)); ?>"/>
      <?php
    }
  }
});
RavanH commented 5 years ago

Hi, yes such a hook is planned but I was considering a simpler solution. Something like: do_action( 'xmlsf_urlset', '__sitemap_type__' ) So one tag (xmlsf_urlset) with a different argument for each sitemap (home, custom, news, post_type etc.) to be able to differentiate between them. Would that work for you?

khromov commented 5 years ago

@RavanH Sure, that works just as well. I implemented the changes (I prefer sitemap over __sitemap_type__ as the arg though, I think it's easier to understand and read). I also updated my example in the original post.

RavanH commented 5 years ago

(I prefer sitemap over __sitemap_type__ as the arg though, I think it's easier to understand and read)

Haha, yes that's exactly what I meant (__sitemap_type__ was just a placeholder) :)

If you could take care of one little type-o in views/feed-sitemap-news.php (a comma inside the hook slug name) then it'll be ready to merge...

khromov commented 5 years ago

Should be good now! 🤞

RavanH commented 5 years ago

Thanks! Will merge :)

One thing to consider though: I think your xmlsf_tags_after action should be simplified to include the original language too. Or at least that is what I understand from https://support.google.com/webmasters/answer/189077 where in the example each time the original language is present along with the alternates.

Something like:

add_action('xmlsf_tags_after', function () {
  if ( ! function_exists( 'pll_languages_list' ) ) return; // to prevent error when Polylang gets deactivated
  foreach ( pll_languages_list() as $language ) {
    $translated_post = pll_get_post( get_the_ID(), $language );
    if ( $translated_post ) {
      ?>
    <xhtml:link rel="alternate" hreflang="<?php echo $language; ?>" href="<?php echo get_the_permalink( $translated_post ); ?>"/>
      <?php
    }
  }
});
RavanH commented 5 years ago

One more question: Is it OK for you if I do the following changes before the final .7 release?

  1. I'd like to rename the hook in the news sitemap to xmlsf_news_urlset as that is more in line with the naming convention for other hooks
  2. I'll probably remove (or rename) the hook in the template views/feed-sitemap.php because that is a sitemap index which does not have an <urlset> but a <sitemapindex> tag...

I am assuming you do not need to add the "xhtml" namespace to the sitemap index or google news sitemap, but if you do, please let me know !

khromov commented 5 years ago

One thing to consider though: I think your xmlsf_tags_after action should be simplified to include the original language too. Or at least that is what I understand from https://support.google.com/webmasters/answer/189077 where in the example each time the original language is present along with the alternates.

You're totally right. Thank you for not only merging but also helping me with my side project in the proce. 😁

I'd like to rename the hook in the news sitemap to xmlsf_news_urlset as that is more in line with the naming convention for other hooks.

No problem!

I'll probably remove (or rename) the hook in the template views/feed-sitemap.php because that is a sitemap index which does not have an but a tag...

Makes total sense!

I noticed that only the feed-sitemap-post_type.php template has the do_action( 'xmlsf_tags_after' ); hook. I don't want to bloat the PR but it would be nice to have a similar hook for the other sitemaps. For example, right now it's not possible to add the same multilingual support to taxonomy sitemaps due to the lack of xmlsf_tags_after hook.

RavanH commented 5 years ago

Yes, good idea... I've just committed these hooks, similar to the xmlsf_urlset hooks. Like <?php do_action('xmlsf_tags_after', 'taxonomy'); ?>