Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#32 | jamesan_19646-32.patch | 650 bytes | JamesAn |
#28 | jamesan_19646-28.patch | 2.58 KB | JamesAn |
#23 | 19646-aggregator-refresh-never-23.patch | 3.41 KB | akahn |
#21 | 19646-aggregator-refresh-never-21.patch | 3.19 KB | akahn |
#20 | 19646-aggregator-refresh-never-20.patch | 3.77 KB | akahn |
Comments
Comment #1
ricabrantes CreditAttribution: ricabrantes commentedbump..
Comment #2
ricabrantes CreditAttribution: ricabrantes commentedComment #3
alex_b CreditAttribution: alex_b commentedLet's add a 'never' option to the Update interval drop down.
Comment #4
alex_b CreditAttribution: alex_b commentedNovice task.
Comment #5
JamesAn CreditAttribution: JamesAn commentedMy 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.
Comment #6
JamesAn CreditAttribution: JamesAn commentedI forgot to pass the 'Never' string through t(). Here's the corrected patch.
Comment #8
JamesAn CreditAttribution: JamesAn commentedI 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?
Comment #9
JamesAn CreditAttribution: JamesAn commentedOops. Changing status.
Comment #11
JamesAn CreditAttribution: JamesAn commentedThat's strange. The patch passed last time I checked. Let's re-test!
Comment #12
alex_b CreditAttribution: alex_b commentedI just applied this patch to a fresh local D7 copy and all tests passed (I used the command line based run-tests.sh script).
Comment #13
JamesAn CreditAttribution: JamesAn commentedYea, 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.
Comment #14
catchAs with the patch for expiry, we should add a constant for AGGREGATOR_NEVER_UPDATE instead of just using 0.
Comment #15
alex_b CreditAttribution: alex_b commentedLet'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.
Comment #16
akahn CreditAttribution: akahn commentedWhen 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?
Comment #17
akahn CreditAttribution: akahn commentedAttached is an updated patch that:
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 adefault_value
for$form['refresh']
below?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 likeNEVER
(as mentioned by catch in IRC) would probably be as expressive as 0, however.Comment #19
akahn CreditAttribution: akahn commentedThe exceptions are from this section:
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?
Comment #20
akahn CreditAttribution: akahn commentedNew 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.
Comment #21
akahn CreditAttribution: akahn commentedNow that #461512: Clean up aggregator_form_feed() is in (thank you, webchick and alex_b), here is a revised version of this patch.
Comment #22
catch$rows indentation seems to be lost in the latest patch.
Comment #23
akahn CreditAttribution: akahn commentedFixed the indentation. This is basically ready to go but let's get one more set of eyes on it.
Comment #24
akahn CreditAttribution: akahn commentedComment #25
Dries CreditAttribution: Dries commentedCommitted 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!
Comment #26
Dries CreditAttribution: Dries commentedComment #27
alex_b CreditAttribution: alex_b commented#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)
Comment #28
JamesAn CreditAttribution: JamesAn commentedThis 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?
Comment #29
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #30
akahn CreditAttribution: akahn commentedThanks for picking this up, JamesAn!
Comment #31
JamesAn CreditAttribution: JamesAn commentedNo prob. ^_^
Should we add the tests and make the CHANGELOG.txt changes in this issue or start a new issue for those tasks?
Comment #32
JamesAn CreditAttribution: JamesAn commentedThis 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.
Comment #33
JamesAn CreditAttribution: JamesAn commentedtestbot?
Comment #34
Dries CreditAttribution: Dries commentedCommitted.