I have several import nodes from the same Feed Importer configuration. I want to override the import_period per node, because sometimes I want to disable fetching for a node or set a different period when the feed should be fetched.

Proposed solution: add a import_period override to FeedsHTTPFetcher.

CommentFileSizeAuthor
#7 interdiff.txt663 bytesklausi
#7 feeds-period-1984982-7.patch2.1 KBklausi
#4 feeds-period-1984982-4.patch2.15 KBklausi
#3 feeds-period-1984982-3.patch2.25 KBklausi
#3 feeds-period-1984982-3-interdiff.txt686 bytesklausi
#1 feeds-period-1984982-1.patch2.21 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Needs review
FileSize
feeds-period-1984982-1.patch2.21 KB

Patch attached.

Status: Needs review » Needs work

The last submitted patch, feeds-period-1984982-1.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
feeds-period-1984982-3-interdiff.txt686 bytes
feeds-period-1984982-3.patch2.25 KB

Added an isset() for the new setting to avoid PHP notices for old configurations.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue summary: View changes
FileSize
feeds-period-1984982-4.patch2.15 KB

Now that we use queue scheduling directly in scheduleImport() of FeedsSource we need to be careful to only invoke schedule if the batch is complete.

twistor’s picture

twistor CreditAttribution: twistor commented
Status: Needs review » Needs work

Hmm... I'm not sure about this. I've been avoiding adding more options at this point.

+++ b/plugins/FeedsHTTPFetcher.inc
@@ -237,6 +255,14 @@ class FeedsHTTPFetcher extends FeedsFetcher {
+    // Apply overridden import period and re-schedule job if necessary.
+    $progress = $source->progressImporting();
+    $config = $source->getConfigFor($this);
+    if (isset($config['import_period']) && $config['import_period'] !== 'importer'
+      && $progress === FEEDS_BATCH_COMPLETE
+    ) {
+      $source->schedule();

If we can only re-schedule when the batch is complete, then form should be disabled if the progress is running. However, it should always be safe to call $source->schedule(), so does this means we introduced a bug in scheduleImport()?

Still not sure, but at a minimum, this would have to be optional. I don't want everyone to get a new form option in everybody's face. This could be done in a contrib module.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

It is not safe to call $source->schedule() now, because we add items to the queue if a batch is in progress. What happened to me in this case is that every import step triggers a source safe which had triggered a schedule() with this patch. So I got 32.000 queue items for one import node with this patch, meaning that the import would run amok and execute all the time with cron.

So maybe we should consider the earlier patch version in #1231332: periodic import imports only one file per cron, which only adds to the queue in the queue worker.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
FileSize
feeds-period-1984982-7.patch2.1 KB
interdiff.txt663 bytes

Just updating the patch here because FEEDS_BATCH_COMPLETE is now a float.