turnermm / news

Dokuwiki plugin for creating external newsfeeds for feed readers
http://www.dokuwiki.org/plugin:news
0 stars 6 forks source link

[PATCH] Add per-feed optional RSS TTL element to allow automatic refresh. #11

Closed cxtal closed 8 years ago

cxtal commented 8 years ago

Hello!

The following patch adds a per-feed optional TTL setting [1] that will add the optional RSS TTL key to the generated XML files. This change will help issues with the XML files being cached as well as instruct automatic RSS updates when to refresh a feed.

After applying this patch the file "newsfeed.ini" can be configured with an extra TTL key, for instance:

[mynews]
title = "The Rabbit's Foot"
description = "Down the Rabbit Hole"
ttl = 60

The being that the resulting XML will have the TTL key in the "channel" section as per the specification:

<channel>
        <title>The Rabbit's Foot</title>
        <link>http://rabbit.org/</link>
        <description>Down the Rabbit Hole</description>
        <language>en-us</language>
        <pubDate>Sun, 03 Jul 2016 12:47:04 +0000</pubDate>
        <ttl>60</ttl>

If useful, please apply! :-)

Kind Regards, Eva

[1] TTL optional parameter, http://www.rssboard.org/rss-specification

turnermm commented 8 years ago

I like this but have one suggestion and one request. First, you assign a value to $newsChannelTtl in both admin.php and feedData.php without first checking to see if the value has been assigned in the $ini_array. I don't think it can be assumed that a $ttl value will be set for each news feed in the ini file. If not, then you need the default, otherwise you will proliferate unassigned index notices in the error log.

The "request" is that once this has been merged, perphaps you can update the plugin page with the new documentation.

cxtal commented 8 years ago

Hello!

You are probably right: I do not know the plugin's internals very well so most likely the $ini_array has to be checked as you say. Would you like me to create a new pull request for that?

I can definitely update the plugin page. :-)

Regards, Eva

turnermm commented 8 years ago

At line 245 of admin.php and 59 of newsfeed,php:

turnermm commented 8 years ago

Sorry closed by mistake

turnermm commented 8 years ago
$newsChannelTtl = isset($ini_array[$which]['ttl']) ? 
                $ini_array[$which]['ttl']: $this->getConf('ttl');

Also, I see a bug at line 251. You have $his instead of $thisin admin.php.

cxtal commented 8 years ago

Got it - new pull request?

turnermm commented 8 years ago

Sounds good.

cxtal commented 8 years ago

Closing #12

turnermm commented 8 years ago

I've merged your additions from the command line as a single commit; otherwise I would have had to merge in two steps, which included the $this/$his error. I cited your github page in the changelog. I made the bare minimum additions to the plugin page; please feel free to update as you see fit. Thanks.

cxtal commented 8 years ago

There is a problem in newsfeed.php, line: 59 in commit #12 :

$newsChannelTtl = isset($ini_array[$which]['ttl']) ? $ini_array[$which]['ttl'] : $this->getConf('ttl');

$this should not be there but I do not know a way around it - $ini_array[$which]['ttl'] is fine though.

turnermm commented 8 years ago

It has been so long since I looked at the plugin that I forgot that the script is external to the plugin proper and doesn't have access to $this. I've added a fix but have temporarily created a new branch for it: https://github.com/turnermm/news/archive/temp.zip.

To see what I've done: https://github.com/turnermm/news/compare/temp?expand=1

If this works for you, I will merge it into the master.

You need this check, because you can't count on all subfeeds having a ttl set in the ini file.

turnermm commented 8 years ago

I created a new pull request: https://github.com/turnermm/news/pull/13 with these changes.