I would love to see a feature in the Drupal aggregator to suspend and re-activate the checking of a feed. Problems with feeds cause issues with cron, yet some of the problematic feeds are corrected some time later. Removing them is to drastic, I think disabling them for the time being would suffice.

Files: 
CommentFileSizeAuthor
#32 jamesan_19646-32.patch650 bytesJamesAn
Passed: 11805 passes, 0 fails, 0 exceptions
[ View ]
#28 jamesan_19646-28.patch2.58 KBJamesAn
Failed: Failed to apply patch.
[ View ]
#23 19646-aggregator-refresh-never-23.patch3.41 KBakahn
Failed: Failed to apply patch.
[ View ]
#21 19646-aggregator-refresh-never-21.patch3.19 KBakahn
Failed: Failed to apply patch.
[ View ]
#20 19646-aggregator-refresh-never-20.patch3.77 KBakahn
Failed: Failed to apply patch.
[ View ]
#17 19646-aggregator-refresh-never-17.patch3.18 KBakahn
Failed: 11226 passes, 0 fails, 38 exceptions
[ View ]
#8 jamesan_19646-3.patch1.6 KBJamesAn
Failed: Failed to apply patch.
[ View ]
#6 jamesan_19646-2.patch3.41 KBJamesAn
Failed: Failed to apply patch.
[ View ]
#5 jamesan_19646.patch1.67 KBJamesAn
Failed: Failed to install HEAD.
[ View ]

Comments

Version:x.y.z» 7.x-dev
Status:Active» Closed (fixed)

bump..

Status:Closed (fixed)» Active

Let's add a 'never' option to the Update interval drop down.

Issue tags:+Novice

Novice task.

Status:Active» Needs review
StatusFileSize
new1.67 KB
Failed: Failed to install HEAD.
[ View ]

My first core patch. ^_^

I added a 'never' option to the Update interval drop down and modified the cron hook query to skip over feeds with this 'never' option.

StatusFileSize
new3.41 KB
Failed: Failed to apply patch.
[ View ]

I forgot to pass the 'Never' string through t(). Here's the corrected patch.

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new1.6 KB
Failed: Failed to apply patch.
[ View ]

I don't know why that one didn't work. I editted the patch manually.

Here's the patch rerolled, but it's identical to the one above except for different timestamps. =\ Maybe it'll go through?

Status:Needs work» Needs review

Oops. Changing status.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

That's strange. The patch passed last time I checked. Let's re-test!

I just applied this patch to a fresh local D7 copy and all tests passed (I used the command line based run-tests.sh script).

Yea, this same things happened to a number of patches I submitted. After passing all tests, they spontaneously failed subsequently, reporting something about "failure to install HEAD".

I've submitted them all for retesting, but the testing platform seems to be down right now: 203 patches queued and 3 free slaves, but no testing is happening. I sent a message through their feedback as boombatower isn't on IRC and chx is afk.

Status:Needs review» Needs work

As with the patch for expiry, we should add a constant for AGGREGATOR_NEVER_UPDATE instead of just using 0.

Let's call it AGGREGATOR_REFRESH_NEVER (instead of AGGREGATOR_NEVER_UPDATE) though - this is more consistent with #60468: Allow aggregator feed items to never be discarded: AGGREGATOR_CLEAR_NEVER.

The convention I followed was [module]_[variable]_[value] .

There should also be a test with this functionality.

When a feed is set to refresh "Never," its "Next update" column on admin/content/aggregator should say "Suspended" or "Never" or something else that indicates that it will not refresh. (Right now it says '0 sec left').

Also, it looks like when a feed is set to refresh "Never," this setting is not the default value when returning to the edit page for a feed. It reverts back to 1 hour. Can anyone verify this?

Status:Needs work» Needs review
StatusFileSize
new3.18 KB
Failed: 11226 passes, 0 fails, 38 exceptions
[ View ]

Attached is an updated patch that:

  • Keeps its "never" setting by only setting it to the default of 3600 seconds if empty($feed->refresh) AND $feed->refresh != 0. There is probably a more elegant way to do this check, maybe by casting the value of $feed->refresh as an integer and then testing if it is greater than or equal to 0. Let me know if this would be preferable. As an aside, what is the use of this check and setting the refresh value to 3600 if there is a default_value for $form['refresh'] below?
  • The "next update" column now says "never" if the feed is set to not update periodically.

It still uses 0 as the key for the "never" setting, because I think that 0 is more expressive of its meaning here than AGGREGATOR_REFRESH_NEVER, for example, would be. Using a generic constant with a name like NEVER (as mentioned by catch in IRC) would probably be as expressive as 0, however.

Status:Needs review» Needs work

The last submitted patch failed testing.

The exceptions are from this section:

<?php
if (!isset($feed)) {
 
$feed = new stdClass();
}
if (empty(
$feed->refresh) && $feed->refresh != 0) {
 
$feed->refresh = 3600;
}
?>

The notice is: Undefined property: stdClass::$refresh

So apparently you can test if the variable is empty but not test its value. I suppose isset() would be a good way to check $feed->refresh, if will return TRUE if it is anything but NULL. But using this here causes the code to not work, the setting gets reset to 1 hour when editing the feed's settings.

What is the purpose of setting up $feed->refresh here? Should this just be done in the default_value of the form element?

Status:Needs work» Needs review
StatusFileSize
new3.77 KB
Failed: Failed to apply patch.
[ View ]

New patch, which rips out the above excerpt of code. Now using default_value for the default value, like it's there for. Let me know if this is a horrible idea and has some consequences that I'm not thinking of.

I also split the columns of the feed table into separate lines for (very slightly) greater readability.

StatusFileSize
new3.19 KB
Failed: Failed to apply patch.
[ View ]

Now that #461512: Clean up aggregator_form_feed() is in (thank you, webchick and alex_b), here is a revised version of this patch.

Status:Needs review» Needs work

$rows indentation seems to be lost in the latest patch.

StatusFileSize
new3.41 KB
Failed: Failed to apply patch.
[ View ]

Fixed the indentation. This is basically ready to go but let's get one more set of eyes on it.

Status:Needs work» Needs review

Committed to CVS HEAD. It would be great if we could write some tests for this though. Setting status to 'code needs work'.

It also feels like we could add an entry to CHANGELOG.txt. Not for this feature in specific, but also for #60468: Allow aggregator feed items to never be discarded, which I committed yesterday.

Thanks all!

Status:Needs review» Needs work

#60468: Allow aggregator feed items to never be discarded introduced a constant for the usage of 0 - we should do this here, too. (See #15)

Status:Needs work» Needs review
StatusFileSize
new2.58 KB
Failed: Failed to apply patch.
[ View ]

This patch replaces 0 with the use of the constant, AGGREGATOR_CLEAR_NEVER.

I've also taken the liberty of moving the AGGREGATOR_CLEAR_NEVER define statement to aggregator.module and removed it from aggregator.process.inc as it was done in #60468: Allow aggregator feed items to never be discarded.

Is it appropriate to merge #60468: Allow aggregator feed items to never be discarded into this issue? Can we create tests and amend the CHANGELOG.txt for both issues in one go without eating any baby kittens?

Status:Needs review» Fixed

Committed to CVS HEAD. Thanks!

Thanks for picking this up, JamesAn!

Status:Fixed» Needs work

No prob. ^_^

Should we add the tests and make the CHANGELOG.txt changes in this issue or start a new issue for those tasks?

StatusFileSize
new650 bytes
Passed: 11805 passes, 0 fails, 0 exceptions
[ View ]

This patch updates the CHANGELOG.txt with the line:
Added options to suspend updating specific feeds and never discard feeds items.

This includes the change made in #60468: Allow aggregator feed items to never be discarded.

I've opened another issue to add tests to cover both these new options at #479348: Aggregator: create tests for new options to suspend feed updates and never discard feed items, so this issue can be closed after we settle on the wording for the CHANGELOG.txt entry.

Status:Needs work» Needs review

testbot?

Status:Needs review» Fixed

Committed.

Status:Fixed» Closed (fixed)
Issue tags:-Novice

Automatically closed -- issue fixed for 2 weeks with no activity.