CommentFileSizeAuthor
#1 feeds-system_alter-2390297-1.patch3.56 KBtwistor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

twistor’s picture

Status: Active » Needs review
FileSize
3.56 KB
MegaChriz’s picture

I've looked at the patch and noticed a few things, but I'm not sure if they are a real problem, so I leave the issue status to "Needs review".

  1. The class "FeedsTestParser" is not listed in the info file (while other classes are), but it apparently doesn't lead to any problems.
  2. feeds_system_info_alter() only iterates over modules that implement hook_feeds_plugins(). This means that Feeds plugins that declare dependency modules that do not implement that hook will not be marked as required. Best example: Taxonomy term processor. The Taxonomy module can still be disabled even if an importer depends on it.
    Just removing the condition !module_hook($file->name, 'feeds_plugins') may have influence on performance as then for every module Feeds recalculates dependencies for all importers.
    Maybe it's better to calculate all dependencies once and cache that into a static variable? Only downside is then that all importers will always be loaded when visiting the module page, even when there are no Feeds extension modules.
  3. Tiny minor thing:
        +++ b/tests/FeedsTestParser.inc
        @@ -0,0 +1,10 @@
        + * Simple test parser,
        

    Comment should end in an period instead of a comma.