Allow Feeds Importers to be edited even when disabled

Great idea! Do note that this introduces an API change. Code may look for a FeedsNotExistingException now to determine if it can do something with the importer in question. See for example drush_feeds_import() in feeds.drush.inc.

Re-uploading the patch here so the testbot would pick it up.

Hm, the tests lack coverage for disabled importers.

Anyway, this has quite some impact on existing code. The changes in the patch will result into content being imported for disabled importers as a lot of code is doing something like this to perform an import:

feeds_source($id, $feed_nid)
  ->existing()
  ->import();

Examples:

• drush_feeds_import() in feeds.drush.inc.
• FeedsHTTPFetcher::request() in plugins/FeedsHTTPFetcher.inc.
• _feeds_queue_worker_helper() in feeds.module.

This could probably be fixed by checking for enabled importers in FeedsSource::existing().

Anyway, the code in feeds.drush.inc would require changes, as right now it says "importer does not exist or is not enabled.":

$importer = feeds_importer_load($importer_id);
if (!$importer) {
  drush_set_error(dt("The importer '@importer' does not exist or is not enabled.", array(
    '@importer' => $importer_id,
  )));
  return FALSE;
}

That looks easier to deal with indeed, but it would still require code changes elsewhere:

try {
  $source = feeds_source($importer_id, $feed_nid)->existing();
}
catch (FeedsNotExistingException $e) {
  // (...)
}

Code like above that is currently only checking for a FeedsNotExistingException, now need to check for FeedsNotEnabledException as well.

I would also turn on the $enabled check by default. Now code would need to call existing() with argument TRUE to also check if the imported is enabled:

// Check if the import exists AND is enabled.
$importer->existing(TRUE);

This would break existing code as well.

I think that we needs tests that covers cases of disabled importers. It would be great if we had tests for the drush code too, but so far I didn't find out how to write for these and have them executed by the testbot.

@wmostrey
Do you like to write tests that cover cases of disabled importers? If you are not familiar with writing tests, would you like to get help with that?

Alright!

The first step in writing the tests is to figure out what to test. One scenario for example is that nothing should be imported for disabled importers, even if configured that imports happen periodically. An other one is that disabled importers should be editable. Can you think of other scenarios? The next step would be to roughly write the steps (in plain text) needed to fulfill the test, for example:

• User with permission to administer Feeds logs in.
• Creates an importer.
• Disables the importer.
• Tries to go the import page.
• The import page does not exist.

You could of course skip this step if you know right from the start how the test should be written (in code).

I haven't experience with writing tests for Drush either. If you would like to go down that path: the test suite for Drush is called 'Unish' and is based on PHPUnit. See drush/tests/Unish. I would use Drush 8 to base the tests on. See also https://github.com/drush-ops/drush/blob/master/tests/README.md

But maybe it's better to start with the Simpletest tests, as there aren't any drush tests for Feeds yet.

Let me know if I'm overwhelming you.

For the case of editing a disabled importer, the test would be best placed in feeds_ui/feeds_ui.test.
For the case of involving periodic imports, tests/feeds_scheduler.test may be a good fit.

For other cases involving disabled importers I'm not sure yet which .test file would fit, so that may need a new file.

Have you made up a list of cases to test? Else the case of editing a disabled importer would be a great one to start with, as that is what this issue is trying to solve. It is unfortunate that there is no test coverage for other cases involving disabled importers, but I think we need them to prevent introducing new bugs.

Great start. Technically, the test for checking if the importer is available on the import page would more belong in the general Feeds tests, as the import page is also available when the Feeds Admin UI module is not enabled. For Feeds Admin UI the only test I think we need is to test if a disabled importer is editable (which is currently not).

About assertText() and clickLink(): I think this is on purpose yes, because in most cases, the "final" text is checked, thus the text for which the translation placeholders have already been replaced. So there is no original translation string to check for. If I remember well, there were also occasions where only a portion of the text was checked. Since tests are always run in English (at least, by my knowledge), this should have no harm.

You are well on your way!

1. +++ b/feeds_ui/feeds_ui.test
   @@ -22,6 +22,19 @@ class FeedsUIUserInterfaceTestCase extends FeedsWebTestCase {
   +    $this->assertNoText('The requested page "/admin/structure/feeds/test_feed" could not be found.');
   

   I see that FeedsUIUserInterfaceTestCase::testDisabledFeeds() is passing. So - in this case - instead of asserting what should not be there, assert what you expect to be there. Actually, most of what happens already in FeedsUIUserInterfaceTestCase::testEditFeedConfiguration(). Maybe create a method called doEditFeedsConfiguration() and then call that method from both test methods?

2. For the scheduler test, it would be better if the disabled importer case would live in its own test method. Ideally, test methods would focus on just one small piece of functionality, rather than trying to test a whole bunch of stuff. In theory, this would also make it easier to port features to the D8 version of Feeds later on, as with separate tests we can get much a clearer view of what the application should be able to do. So FeedsSchedulerTestCase::testScheduling() is probably a bad example: I would need to read the whole test method to get an idea of what it should all be able to do.

That's quite good. :)

Review

A few remarks:

1. I think that the methods doEditFeedsConfiguration() and initSyndication() should live in the test classes where they are called, instead of feeds.test, in order to keep the related test code together. If it would be very generic code, feeds.test would be a good place.
2. I think that the testDisabledScheduling() needs an extra call to cronRun(), as the other scheduling test has that as well. I don't remember the reason why there are two cron run calls though, maybe it is explained in one of the other tests.
3. Then some nitpicks: I would keep the name testScheduling(), rename testDisabledScheduling() to testSchedulingWithDisabledImporter() and rename testDisabledFeeds() to testEditDisabledImporter().

Drush tests

I've made an attempt to write Drush integration tests for a disabled importer. See attached patch. Currently, when running these tests, Drush downloads the latest stable release of Feeds (7.x-2.0-beta3), which isn't very useful: it will then only test the Drush integration code as present in the stable release. I've yet to figure out how I can let Drush to pick the local copy of Feeds, so Drush tests can run using a patched version of Feeds.
I also don't know if the drupal.org testbot can support these Drush tests.

To run the Drush tests:

1. Download Drush 8.
2. Install Drush using Composer with dev dependencies.
3. On the command line, execute a command like the following:
   

   UNISH_NO_TIMEOUTS=1 UNISH_DRUPAL_MAJOR_VERSION=7 /path/to/drush/vendor/bin/phpunit --configuration /path/to/drush/tests /path/to/feeds/tests/drush
   

   Replace '/path/to' with the appriopiate path to the directory in question.

The Drush integration tests patch

The Drush integration tests for Feeds are updated in the attached patch! Thanks @wmostrey for figuring out how to the execute the tests against the local copy of Feeds. Unish happens to install a complete Drupal website at a different location on the hard drive. In my case that is:

/private/var/folders/6g/qmxgmns11g1_6k1l721tnggc0000gs/T/drush-sandbox/web/

The Feeds directory and its contents are now copied to this folder.

How to run the Drush tests

The instructions for running the Drush tests as explained in #18 were not entirely clear. Here are some more detailed instructions:

In order the run Drush integration tests, Drush itself needs to be installed with its dev dependencies. Furthermore, the phpunit version that comes with Drush should be used for running the tests (instead of a globally installed phpunit), as that one has proven to be compatible with the Drush tests.

1. Git clone of Drush.

   git clone --branch 8.x https://github.com/drush-ops/drush.git
       cd drush

2. Install Drush with dev dependencies using Composer (while being inside the Drush folder).

   composer install
       

   And ensure that the following text is displayed:

   Loading composer repositories with package information
   Installing dependencies (including require-dev) from lock file

   Especially note that Composer says 'including require-dev'. This means that the Drush dev dependencies are installed (including phpunit).

3. Execute a command like the following:
   UNISH_NO_TIMEOUTS=1 UNISH_DRUPAL_MAJOR_VERSION=7 /path/to/drush/vendor/bin/phpunit --configuration /path/to/drush/tests /path/to/feeds/tests/drush
   Replace '/path/to' with the appropriate path to the directory in question. Also be sure to point to the phpunit version that comes with Drush.
   So if Drush is installed in /users/megachriz/drush and the Drupal 7 site is installed in /users/megachriz/Sites/drupal7:

   UNISH_NO_TIMEOUTS=1 UNISH_DRUPAL_MAJOR_VERSION=7 /users/megachriz/drush/vendor/bin/phpunit --configuration /users/megachriz/drush/tests /users/megachriz/Sites/drupal7/sites/all/modules/feeds/tests/drush
       

I've added these instructions to the README in the attached patch.

What's next?

I should look at the patch in #19. Assuming the tests in there are alright, the next step is to work on a fix. The last proposal for the fix was in #5, for which some remarks were made in #6.

@wmostrey
The tests look good. Some nitpicking: code comments should end with a dot.

Do you want to complete this issue by improving the fix, combined with all the tests from this issue?

Okay, great!

This is a though one. I've been thinking about it for a while. I'm not completely sure about adding a parameter to the existing() method.

Let's analyze. We have two situations to deal with:

1. Code that is calling the existing() method.
   This code is expecting a FeedsNotExistingException if the importer or source should not be used for import or clear tasks.
2. Code that is calling feeds_importer_load().
   Code calling this method expect to retrieve an importer object, but only if it exist and is enabled.

The existing() method

Code that is calling this method usually is in the following pattern:

try {
  $source = feeds_source($importer_id, $feed_nid)->existing();
}
catch (FeedsNotExistingException $e) {

}

So code would break if the existing() method would suddenly return a FeedsNotEnabledException. In the first place because the code isn't prepared for such situation. Secondly, because you cannot catch two different types of exceptions at the same time (unless one is a parent of the other).
Possible ways to fix this:

• Let FeedsNotEnabledException extend FeedsNotExistingException and introduce a different type of exception for if an importer truly doesn't exist and which also extends FeedsNotExistingException.
• Do not introduce a FeedsNotEnabledException class at all and fix the problem in a different way.

feeds_importer_load()

If we change the (default) behavior of this method to return a feeds importer even if this is disabled, this could possibly break code that is expecting an usable importer (existing and enabled). However, I've seen code in other modules (like my feedspreview module) that actually will need to act on disabled importers once it becomes possible to edit disabled importers. Since it is also advertised as menu callback, I think we can justify an API change here.

To wrap up

Now that I've been through this analyze process I think we need two extra methods on the FeedsConfigurable class:

1. One that returns if the object exists: doesExist().
2. One that returns if the object is enabled: isEnabled().

Both methods would return a boolean (TRUE or FALSE). The existing existing() method would then call both methods and throw a FeedsNotExistingException for each method returning FALSE.
feeds_importer_load() would then load the importer and only return it if doesExist() returns TRUE.
In this scenario there would be no need for introducing a FeedsNotEnabledException class, I think.

What do you think?

Okay, great! Looks good.

A few remarks:

• For consistency, FeedsSource should probably override the new doesExist() method.
• The drush code needs an update: now that feeds_importer_load() can return a disabled importer, the text "The importer '@importer' does not exist or is not enabled." no longer is true. Furthermore, code should abort when the importer is disabled. So probably we need two texts here: one for not enabled and one for not existing. Abort in both cases. Changes need to be made in drush_feeds_import() and drush_feeds_clear().
• Minor: the implementations of the methods doesExist() and isEnabled() could be a single line of code.

Thanks, wmostrey. Code-wise this looks good!

I'll look into making FeedsSource::doesExist() consistent.

This adds two methods to the FeedsSource object:

• hasValidConfiguration(): checks if the source configuration is valid. FeedsSource::existing() uses this to throw a FeedsNotExistingException with a specific message.
• doesExist(): checks if both the importer and the source exist.

I will do some manual tests in a few days and if I found no further issues, then I'll commit the patch!

Thanks for all your work, wmostrey!

Forgot to include the interdiff.

Just noticed that the drush tests from #20 were missing. Added them.

I looked at all the code changes once more, executed the drush tests, and performed the following manual tests:

• Importing/clearing through drush commands with both enabled/disabled importers.
• Disabled an importer that existed in db and try to edit it.
• Disabled an importer that existed in code and try to edit it.
• Configured periodic import of an importer to run as often as possible, with update existing and 'Skip hash check' setting enabled. Ran cron a couple of times to confirm items were imported via this importer. Then disabled the importer and ran cron again. Nothing got imported anymore.

And I noticed two small things:

• When trying to perform an import using the drush command 'feeds-import' and the importer is disabled, the returning text says the following:
  

  The importer 'my_importer' is not enabled.

  This is good, however I thought it would be useful to add how it can be enabled. Now it reads:

  The importer 'my_importer' is not enabled. You can enable it with the command 'drush feeds-enable my_importer'.

• feeds_importer_load() previously catched a InvalidArgumentException. I found out that an exception like this could happen on multilingual sites or when Tamper configuration was not properly cleaned up after the deletion of an importer. See #1887632: Exception: Empty configuration identifier. in FeedsConfigurable::instance() (line 55 of feeds/includes/FeedsConfigurable.inc).. So I added that exception catching back.

If this patch still passes tests, then I'll commit it!

Committed #34.

Thanks again for all your work, wmostrey!