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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ricabrantes’s picture

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

bump..

ricabrantes’s picture

Status: Closed (fixed) » Active
alex_b’s picture

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

alex_b’s picture

Issue tags: +Novice

Novice task.

JamesAn’s picture

Status: Active » Needs review
FileSize
1.67 KB

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.

JamesAn’s picture

FileSize
3.41 KB

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.

JamesAn’s picture

FileSize
1.6 KB

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?

JamesAn’s picture

Status: Needs work » Needs review

Oops. Changing status.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review

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

alex_b’s picture

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).

JamesAn’s picture

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.

catch’s picture

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.

alex_b’s picture

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.

akahn’s picture

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?

akahn’s picture

Status: Needs work » Needs review
FileSize
3.18 KB

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.

akahn’s picture

The exceptions are from this section:

 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?

akahn’s picture

Status: Needs work » Needs review
FileSize
3.77 KB

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.

akahn’s picture

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

catch’s picture

Status: Needs review » Needs work

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

akahn’s picture

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

akahn’s picture

Status: Needs work » Needs review
Dries’s picture

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!

Dries’s picture

Status: Needs review » Needs work
alex_b’s picture

#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)

JamesAn’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

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?

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

akahn’s picture

Thanks for picking this up, JamesAn!

JamesAn’s picture

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?

JamesAn’s picture

FileSize
650 bytes

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.

JamesAn’s picture

Status: Needs work » Needs review

testbot?

Dries’s picture

Status: Needs review » Fixed

Committed.

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

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