RavanH / xml-sitemap-feed

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

Google News: better title filters, introduce real xmlsf_news_tags_after #36

Closed rubas closed 4 years ago

rubas commented 5 years ago

Careful This are necessary changes, but can break current code!


Surprisingly the same filter was used for the publication:name and the news:title. I changed this and renamed the filters in line with rest.

If you want to add <image> to the <url>, those have to be defined outside <news>. I introduced a proper action xmlsf_news_tags_after and renamed the old one to xmlsf_news_tags_before.

RavanH commented 5 years ago

Hi @rubas thanks for your contribution. I've got some remarks:

  1. Using a different filter hook for sitemap and news sitemap title makes sense but you'll have to re-add some basic filters then. Right now, in the xml-sitemap-feed/controllers/functions.php there is used for both normal and news sitemaps:

        // common sitemap element filters
        add_filter( 'the_title_xmlsitemap', 'strip_tags' );
        add_filter( 'the_title_xmlsitemap', 'ent2ncr', 8 );
        add_filter( 'the_title_xmlsitemap', 'esc_html' );
  2. I see your point for the need of an extra action hook in the news sitemap but I'm not sure about the naming. xmlsf_news_tags_before is not really before... Maybe xmlsf_news_tags_inner or xmlsf_news_tags_bottom? Plus, moving xmlsf_news_tags_after it will indeed break compatibility for those that are already using the hook. Not sure how to warn users to adapt their code...

So what I'd suggest is leave the current hook in place, and add an existing one after the closing </news:news> tag, with a new parameter:

<?php do_action( 'xmlsf_tags_after', 'news' ); ?>

You can then use that hook with an action like:

function my_additional_tags( $sitemap ) {
  if ( 'news' == $sitemap ) {
    // echo your news tags here
  }
  // you can echo tags for other sitemaps by polling for home | post_type | taxonomy | custom
}
add_action( 'xmlsf_tags_after', 'my_additional_tags' );
rubas commented 5 years ago
  1. Thanks. Missed those.

  2. Agree with inner as the best naming.

Normally breaking changes are noted in the changelog of the plugin. I do believe it would make sense to clean up the naming a little.

rubas commented 5 years ago

And if we are on it. It would make a lot of sense to add a composer.json to the repository.

It allows people to manage the plugin directly via composer. It's not necessary to have it published to packagist for that. Just add this to the repositories.

    {
      "type": "vcs",
      "url": "git@github.com:ravanh/xml-sitemap-feed.git"
    }
RavanH commented 4 years ago

Hi @rubas just to let you know: I went with xmlsf_news_tags_inner for the upcoming release. Your news title filters have been implemented as well as the composer.json file. Thanks again for your contributions :)