Hm hopefully this isn't a red herring. This site has a bit of a sketchy history. Anyway, trying to install Feeds 7.x-2.0-beta1. Previous version was 7.x-2.0-alpha7+18-dev with two patches (one for http://drupal.org/node/661606#comment-6857198 and one for http://drupal.org/files/feeds-nocreate-1286298-7.patch).

Upon trying to run the associated database updates, fails on

PHP Fatal error: Call to undefined function feeds_importer_load_all() in /var/www/[mysite]/sites/all/modules/feeds/feeds.install on line 699
Fatal error: Call to undefined function feeds_importer_load_all() in /var/www/[mysite]/sites/all/modules/feeds/feeds.install on line 699
Drush command terminated abnormally due to an unrecoverable error. [error]

Not sure how to get out of this. This is a dev site so I'm not panicking. Yet.

Comments

twistor’s picture

Status: Active » Postponed (maintainer needs more info)

This is probably pretty easy to fix, but I would love to reproduce it because I've heard of this before.

What version of Drush are you using?

memcinto’s picture

drush 5.7.

twistor’s picture

Status: Postponed (maintainer needs more info) » Active
agerard’s picture

same problem here... also a dev site, but would like to track it down and fix before I find it on a production site. I'm using drush 6.2.0

twistor’s picture

One thing that can cause this problem is if feeds is disabled. I think we need to re-work the update.

memcinto’s picture

Note that I *think* originally when I updated feeds on my dev site, I thought the db updates were in a loop, because it kept repeating "Performed update: feeds_update_7211" over and over, but I didn't realize that it would need to run that multiple times for each related importer mapping. So I think I CTRL-C'd the update. Then of course it was messed up, then I tried disabling feeds, then etc. gradual but total meltdown.

Today I wiped my dev site and synced it from prod, and this time installing the new version of Feeds worked without any errors.

twistor’s picture

Version: 7.x-2.0-beta1 » 7.x-2.x-dev
Status: Active » Needs review
StatusFileSize
new1.8 KB

This should fix the issue, but it's a terrible hack.

inversed’s picture

I haven't tried the patch but enabling Feeds before running the update for 7211 fixed the problem for me.

megachriz’s picture

If it is a "terrible hack", then this could at least use tests. I wrote a test that tests all database updates since 7.x-2.0-alpha7. More about that later. First into the review of the solution.

Review

I didn't test the patch manually, but the automated tests (see further) prove that it works as expected.

  •   +++ b/feeds.install
      @@ -696,14 +696,28 @@ function feeds_update_7211(&$sandbox) {
      +    foreach (array('feeds', 'ctools') as $module) {
      ...
      +      module_enable(array_reverse($sandbox['enabled_modules']), FALSE);
      

    I wonder why the array_reverse() is needed here. Couldn't 'feeds' and 'ctools' just be defined in reverse order or is there is a special reason for it?

  • An alternative to this solution could be to throw a "DrupalUpdateException". This will report to Drupal that the update failed and the update will remain open until the user resolves the issue (in this case, enable the Feeds module). I thought about making a patch first for this solution, but since the patch in #7 already works, I left this out. This would have been the code:
    if (!function_exists('feeds_importer_load_all')) {
      // Feeds should be loaded first.
      throw new DrupalUpdateException(t('Enable the Feeds module to perform this update.'));
    }
    

Tests

Based on the instructions from https://www.drupal.org/node/1429136 I wrote an update path test. The test case contains two methods:

  • One that tests updates with the Feeds module enabled. It also checks if the updates did their work correctly.
  • One that tests updated with Feeds and Ctools disabled. This will check if feeds_update_7211() was done correctly and if Feeds and Ctools remain disabled after the updates are done.

I chose the Feeds 7.x-2.0-alpha7 version as a starting point to test updates from because that is the oldest version I can remember to have used. And also because the one who started the issue mentioned it.

FeedsConfigurable not found error

During writing the test I kept getting the following error when running the tests:

Class 'FeedsConfigurable' not found

To fix this, I modified feeds_importer_load_all() to check for this class before it continues. Since it also checked if the function ctools_include() exists, I thought it was okay to add an extra check there. Strictly, this should be handled in an other issue, but I wanted to get going with tests for this issue.
I noticed that people got this error in the past too during updating and they were told to do a registry rebuild and then continue. With this addition that would no longer be needed. See these (already closed) issues:
#1177844: Fatal error with FeedsConfigurable.inc
#2297917: Fatal error: Class 'FeedsConfigurable' not found in feeds.module on line 898
#884638: Feeds_ui ERROR: Class 'FeedsConfigurable' not found
#857246: Fatal error: Class 'FeedsConfigurable' not found

Bonus effect, side effects

As a bonus, future update functions will now also be evaluated. This is because the update path tests performs all database updates. Thus if one is added via a patch, the update path test will execute that one as well. There is also a negative side: if structural changes are made that touches things done in earlier updates (that will be the bundle setting, the file:uri target and the cache_feeds_http table), the update path test may fail. An other negative side effect is that the tests performs updates from Drupal Core and Ctools as well. Should there be a bug in the update functions in an official release from these two projects, then the update path test will fail and Feeds will appear to be broken. Should this occur, then the solution is easy: update the schema and schema version of these modules in the test and the database updates will appear to Drupal to already have ran.

Patches

  • One with tests only, to confirm the bug.
  • One with tests + the fix from #7.

The last submitted patch, 9: feeds-feeds_importer_load_all-2537290-9-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: feeds-feeds_importer_load_all-2537290-9.patch, failed testing.

twistor’s picture

Wow, that's a lot of work. It's definitely a good idea.

Regarding the hack, I think trying to change importer configuration in an update is hacky in general. There's not way to ensure everything will get updated.

In this specific case, the uri should have never been added, but that's too late to change now.

megachriz’s picture

Thanks. For some reason though the tests fails on updating the bundle config for the node processor (the test passed locally). The paths of were the contrib modules are installed are also hardcoded to what the testbot uses during the tests, namely "sites/default/modules/", so that makes the test fail locally when Feeds is installed on an other location. I see I can fix that.
I put reviewing #2542416: Allow sources and targets to be defined but hidden. on my todo list. I also still am working on tests for the multilingual issue.

megachriz’s picture

Status: Needs work » Closed (won't fix)

The update feeds_update_7211 has been removed in #2542416: Allow sources and targets to be defined but hidden., which makes this issue obsolete.

The tests for testing Feeds database updates are interesting to have, but that should be an other issue.

A new release is planned for next week.