Drush command to import all feeds

@Odai Atieh
Thanks for the patch! Do you also want to provide test coverage for this new command? You can add the test to feeds/src/Functional/Commands/FeedsDrushCommandsTest.php.

As this module has an issue for fixing coding standards let's not introduce any more here. See the test results for coding standards error messages.

Otherwise the patch looks good, including test coverage. I would happily mark RTBC once the testbot clears coding standards.

No worries @Odai Atieh, the comment is directed to anybody who has the time and inclination to sort it out, not just you. We all get busy :)

The coding standards fails in the test results page do come from the patch. Here's how:

1. +++ b/src/Commands/FeedsDrushCommands.php
   @@ -213,6 +213,48 @@ class FeedsDrushCommands extends DrushCommands {
   +    $feeds = \Drupal::entityTypeManager()
   +        ->getStorage('feeds_feed')
   +        ->loadMultiple($entityQuery->execute());
   

   line 241 - Object operator not indented correctly; expected 6 spaces but found 8

2. +++ b/tests/src/Functional/Commands/FeedsDrushCommandsTest.php
   @@ -192,6 +192,31 @@ class FeedsDrushCommandsTest extends FeedsBrowserTestBase {
   +    $feed1 = $this->createFeed($this->feedType->id(), [
   +        'title' => 'Foo',
   +        'source' => $this->resourcesPath() . '/rss/drupalplanet.rss2',
   +    ]);
   

   201 Array indentation error, expected 6 spaces but found 8
   202 Array indentation error, expected 6 spaces but found 8

3. +++ b/tests/src/Functional/Commands/FeedsDrushCommandsTest.php
   @@ -192,6 +192,31 @@ class FeedsDrushCommandsTest extends FeedsBrowserTestBase {
   +    $feed2 = $this->createFeed($this->feedType->id(), [
   +        'title' => 'Bar',
   +        'source' => $this->resourcesPath() . '/rss/googlenewstz.rss2',
   +    ]);
   

   206 Array indentation error, expected 6 spaces but found 8
   207 Array indentation error, expected 6 spaces but found 8

I've been meaning to try this patch out, but each time something else comes up. These indentation errors seems easy to fix, I could even fix these on commit as soon as I've time to look at the patch in detail. But I'm not against it if these indentations errors are fixed in a new patch. :)

Fixed the indentation issues. Please review.

Thank you

Great work everyone.

We could probably re-word some of the comments to be a little clearer but I don't know the maintainer's appetite for that kind of nit-picking on this module.

I've tested the patch in #11, works mostly great.

I tested the following:

• Import all feeds and checked if disabled ones got skipped.
• Import all feeds, including disabled ones.
• Import all feeds for a specific type.
• I tried to import all feeds for two specific types using drush feeds:import-all foo bar, but that resulted into the error "Too many arguments".

To support specifying multiple feed types, I adjusted the code a bit. Then I noticed that the feeds weren't always imported in the order I specified the feed types in the command. For example, in some cases when executing drush feeds:import-all foo bar, a feed of type "bar" got imported first and a feed of type "foo" second. While I would like it to be the other way around.
So I added some more code to order the feeds when more than one feed type was passed to the command.

Then I looked at the tests and noticed this could use some more test coverage:

• I expanded the existing "testImportAllFeeds" by adding a disabled feed and one more feed type.
• Added a test for importing all feeds including disabled.
• Added a test for importing all feeds of one type.
• Added a test for importing all feeds of two types.

I also found it useful to know which feeds got imported along the way, so added a message to the command 'Starting import of feed "Foo" (id 1).'.

Setting back to "Needs review", but I do plan to commit this in a couple of days, if tests are still passing.

Oops, I've been reviewing bugs solidly for a few weeks and forgot that feature requests need a bit more love when it comes to fleshing out requirements, thanks for taking that on @MegaChriz.

If the "starting import of X" message is important to us it may benefit from some test coverage but it's not the kind of thing that regresses easily.

I have no further feedback on the code, but after that faux pas I'm reluctant to push it back to RTBC.

Committed #13 with a few small changes that don't influence the functionality (some code cleanups and one additional usage example). Thank you all who participated in this issue!

@darvanen
No problem in not having spot all details this time, I value your input and review!

@Odai Atieh
Thanks for wanting to looking into improving the performance. :) startBatchImport uses Drupal's batch API: it calls batch_set(). Normally, this triggers a progress bar in Drupal's UI. I haven't tested if this batch API also can be used with drush.

I know drush also has a progress bar feature, though I'm not sure if that would improve performance per se - it just reports progress. But that surely would be nice to have for Feeds, especially for large imports.

Thanks, setting the issue to "Needs review" so that hopefully the testbot runs test with the patch.

I did not see the drush progress bar I hoped for, but I committed #20 anyway, assuming it improves the import processes.

Thanks for the update. Since I want to create a new release of Feeds today, I just reverted the change for now.

Feedback on your patch: it is highly discouraged to import data for a locked feed. When a feed is locked, an import for it is supposed to be still in progress. Unlocking a feed should only happen if you are certain that the import abruptly ended. So because of that I think it is a bad idea to add an option for unlocking any feed. Ideally the Drush command should skip each feed that is locked and continue to the next one.