Show import period in feed form, add configurable override option

Related D7 issue: #1984982: Allow sources to override the import period

It is an interesting feature request.
I'll try to work on it.
However:
1. Add an option on the feed type settings form to show the import period on the feed form.
2. Expose the import period setting on the feed form accordingly.
3. Add an option on the feed type settings form to allow chanding the import period on the feed form.
4. Add a permission to change the feed import period on the feed.

Option 3, I don't think is necessary. Either we are showing this option or not.

So, I've implemented a generic solution and attached the patch.
I skipped:

Add a permission to change the feed import period on the feed.

There are no separate field permissions on the feed edit form, so, better to keep it as it was designed.
Example of the configuration:

Attached a new patch.

Hey Youri van Koppen!

It seems I need your help with
27.863s Drupal\Tests\feeds\Functional\Update\UpdateCustomSourcesUpdateTest 0 passed, 1 failed, 1 log(s)
28.017s Drupal\Tests\feeds\Functional\Update\UpdateNonExistentActionUpdateTest 0 passed, 1 failed, 1 log(s)

I was not able to fix them locally. And I'm open for your PR review.

Attached a new patch.

@andriy khomych
Thanks for your work on this issue, I think I'll look at this after DrupalCon Vienna (which I'm attending).

Thanks, changes look good to me, the only thing missing is now the test fixing.

Hey Youri van Koppen!

I would appreciate your help with tests.
Let me know if I need to provide further improvements.

I see in that the two failed update tests are using the 'feeds-8.x-3.0-alpha6-feeds_installed.php' fixture, in which the tables 'feeds_feed' and 'feeds_subscription' are not installed. These table are installed in the fixture 'feeds-8.x-3.0-beta1-feeds_installed.php'. So I've added the tables to 'feeds-8.x-3.0-alpha6-feeds_installed.php' as well now. Let's see if that fixes the tests.

I hope to review/test your code soon! I would like to have that done by Sunday.

Hey Youri van Koppen!

Thank you! I appreciate your help!
Looking forward to your review.
It seems we have only to fix phpunit (previous minor) and phpunit (previous major) pipelines.
I think I can check over it after the MR review.

Thanks for your work! I've added some comments on the MR in the GitLab interface.

Besides that, here is more from my review:

• Feed form
  1. I think that the term "importer" is not used in Feeds 8.x-3.x. So instead of "Use importer setting", would "Use the feed type default: @default" be better? It would be great if it is displayed what the default import period is. Since this cannot be directly done in the static method basePeriodicImportFieldDefinition() because of lacking context, I think that the label should be "Use the feed type default" there. And an override of the label (that also tells what the default import period is) could then be best implemented in \Drupal\feeds\FeedForm::form().
• Feed View
  2. When I have an import period on the feed, if I then disable "Allows import period per feed" on the feed type, the import period is still displayed on the page. I think that as soon as you disable "Allows import period per feed", the field value should no longer be displayed.
• Import testing
  I tested the import setting on the feed. When I set it on the feed to "As often as possible", while the feed type has "Every 1 hour", an import does indeed happen on every cron run. So this works!
  I also tested setting the import setting on the feed first to "As often as possible" and then I disabled "Allows import period per feed". The first time I ran cron, an import happened (which is okay I think) and when running cron again a few times no more imports happened. So this works good as well.
• Additional tests
  I think it can be useful to have tests also for the following:
  1. A functional test that ensures that the "Import period" field is displayed on the feed form when "Allows import period per feed" is enabled on the feed type.
  2. A functional test that ensures that the "Import period" field is not displayed on the feed form when "Allows import period per feed" is disabled on the feed type.

  I have suggested a few more tests on the MR.

Hey Youri van Koppen!

Feed form
I think that the term "importer" is not used in Feeds 8.x-3.x. So instead of "Use importer setting", would "Use the feed type default: @default" be better? It would be great if it is displayed what the default import period is. Since this cannot be directly done in the static method basePeriodicImportFieldDefinition() because of lacking context, I think that the label should be "Use the feed type default" there. And an override of the label (that also tells what the default import period is) could then be best implemented in \Drupal\feeds\FeedForm::form().

Yes, and I used "Use the feed type default" due the lack of context, feel free to verify it.

Feed View
When I have an import period on the feed, if I then disable "Allows import period per feed" on the feed type, the import period is still displayed on the page. I think that as soon as you disable "Allows import period per feed", the field value should no longer be displayed.
Import testing
I tested the import setting on the feed. When I set it on the feed to "As often as possible", while the feed type has "Every 1 hour", an import does indeed happen on every cron run. So this works!
I also tested setting the import setting on the feed first to "As often as possible" and then I disabled "Allows import period per feed". The first time I ran cron, an import happened (which is okay I think) and when running cron again a few times no more imports happened. So this works good as well.

Thanks for testing, glad to hear that it works.

Additional tests
I think it can be useful to have tests also for the following:
A functional test that ensures that the "Import period" field is displayed on the feed form when "Allows import period per feed" is enabled on the feed type.
A functional test that ensures that the "Import period" field is not displayed on the feed form when "Allows import period per feed" is disabled on the feed type.
I have suggested a few more tests on the MR.

Added 2 more tests.
Also, I was not able to fix failing pipelines, and looking forward to your support:
- https://git.drupalcode.org/issue/feeds-3363508/-/jobs/7097431
- https://git.drupalcode.org/issue/feeds-3363508/-/jobs/7097430
- https://git.drupalcode.org/issue/feeds-3363508/-/jobs/7097432

I've done a new review!

Failing tests
I've checked the failing test and added $this->printMessages(); right before the failing line. printMessages() is a method defined in FeedsCommonTrait and can reveal why an import failed by printing the error messages that were made during the import.

The error message said:

The content test 1 failed to validate with the following errors:

• field_file.0: You do not have access to the referenced entity (file: 2).
• field_image.0: You do not have access to the referenced entity (file: 1).

I found out that the failing tests are related to these pieces of code in Feed::basePeriodicImportFieldDefinition().

$cron_required = [
  '#type' => 'link',
  '#url' => Url::fromUri('https://www.drupal.org/docs/user_guide/en/security-cron.html'),
  '#title' => t('Requires cron to be configured.'),
  '#attributes' => [
    'target' => '_new',
  ],
];

->setDescription(t('Choose how often a feed should be imported.') . ' ' . \Drupal::service('renderer')->renderRoot($cron_required))

Commenting out the setDescription() call made the test pass on Drupal 10. I think rendering this element in a Kernel test doesn't work or at least it is causing issues.

I propose to move the description of the field to FeedForm::form(). And in the form code, the call to renderRoot() will not be needed.

Hey Youri van Koppen!

Regarding:

Commenting out the setDescription() call made the test pass on Drupal 10. I think rendering this element in a Kernel test doesn't work or at least it is causing issues.

I propose to move the description of the field to FeedForm::form(). And in the form code, the call to renderRoot() will not be needed.

I've reworked the description and updated period logic into a common helper to reuse it in common places.
Looking forward to your review!

Thanks for the update! I hope to review your work this Thursday or else Saturday.

I think that this is good to go! Changes that I made:

• Expanded description of CronTest::testImportPeriodPerFeed() a bit more and also added a few more comments in the test.
• When creating or editing a feed, I noticed the UI showed an option called "- None -". I think this was confusing because that option did essentially the same as "Use the feed type default", so I removed that option in FeedForm. I also set the default value of the field to "Use the feed type default" in case no value was set yet.

Is it good to go for you too?

I think that this is good to go! Changes that I made:

Expanded description of CronTest::testImportPeriodPerFeed() a bit more and also added a few more comments in the test.
When creating or editing a feed, I noticed the UI showed an option called "- None -". I think this was confusing because that option did essentially the same as "Use the feed type default", so I removed that option in FeedForm. I also set the default value of the field to "Use the feed type default" in case no value was set yet.
Is it good to go for you too?

Thanks, Youri van Koppen!
I'm fine with this; feel free to merge it.
I've also attached an updated patch.

@andriy khomych
Thanks for confirming! Also thanks for your contribution and your patience to get this issue resolved. I scheduled the MR for merging.

It seems GitLab creates a patch only for the first commit.
I added an updated patch based on the diff.

I added a patch for the 3.0 version (it does not include tests)

Attached the updated 3.0 version patch with a fix for the interval set.