Dependency injection for \Drupal calls in classes

Seems like tests need to be updated as well?

I think that FeedsDrushCommands needs to implement an additional interface, ContainerInjectionInterface, if I'm correct:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Dependenc...

For FeedsDrushCommands I created a separate issue (#3259633), as the entity_type.manager should be also declared in drush.services.yml.
Still needs work for other classes, though.

I'll work on this!

There's my patch, I didn't fix the FeedsDrushCommands because it has already been fixed in #3259633: Dependency injection for FeedsDrushCommands class

Hey everyone!

I worked on this patch and there were some errors in tests, but I believe that happened because some changes made in patch, so maybe will be need to change tests, but it isn't scope of this issue.

I reviewed #8's patch and worked a little bit on it, as it has an error which was breaking the site (related to DefaultEntityProcessorForm). Also included User::load service dependency injection.

I sent the same patch here: #2939688-47: Fix the errors/warnings reported by PHP_CodeSniffer as a part of Coding Standards Fix.

Also didn't touch FeedsDrushCommands because it has already been fixed in #3259633.

@bruno.bicudo
Thanks for the update. I changed the variable names and code comments a bit. Otherwise it looks good!

Compared to the patch in #8, I see that for some files dependency injection fixes are no longer included. So after committing this patch, I'll leave this issue open for that.

I see I forgot to update referencing the right class at the top of the file. Tests didn't fail upon that, so that means DefaultEntityProcessorForm isn't covered by tests. It would be good to add these.

I'll try to work on this.

I worked on a simple test which creates and edits a test feed.

After revision i'll try to work on extra Dependency Injections on the module.

Needs review :)

@bruno.bicudo
Thanks for working on the test! It looks mostly good. Things I would change/add:

1. After the feed type is saved in the UI, it would be a good idea to load the feed type and check if it was saved with the expected values:
   

   $feed_type = FeedType::load('test_feed_type');
   $this->assertEquals('Test Feed type', $feed_type->label());
   $this->assertEquals('A test feed', $feed_type->description);
   

   For checking the processor configuration, you first would need to call a method to get the processor plugin and than another to get the processor plugin configuration. If I'm not mistaken, you would get it this way (not tested):
   $feed_type->getProcessor()->getConfiguration()

2. Some comments, names and values used in the test suggest that it is creating a feed, while in fact a feed type gets created.
3. Ideally, each test method tests one specific feature or scenario. So I would prefer to have the edit feed type test in a separate test method. In that test, start with creating the feed type programmatically:
   $feed_type = $this->createFeedType();

Thanks so far!

I'll work on the pointed changes. Thanks for the feedback :)

I worked on what was pointed on #17.1 and #17.2.

I also tried to create the edition test based on what's stated on #17.3.

Needs review :)

@bruno.bicudo
Thanks for working on the test! I went through it and made a few changes to the code comments and naming. The class is now called "FeedTypeTest". For #2904668: Explain why a feed type cannot be deleted in the UI I like to add test coverage for deleting feed types too, so I needed a more generic class name.
If the test passes, I'll commit the patch. Then #14 can be retested.

Hm, maybe in #19 the tests were not executed because there was a namespace error in the patch. I made some corrections.

Committed #22. Now #14 need to be retested by the testbot.

Committed #14. Leaving open for remaining changes in #8. That one needs a reroll now (with the changes for DefaultEntityProcessorForm excluded, since these are now committed).

I'll work on it.

I rerolled #8 and made a few changes:

- config.factory service injection for FeedSubscriber must be passed as an argument via feeds_test_multiple_cron_runs.services.yml (arguments: ['@config.factory']). That's why CronRun test was stating an error.

- I also edited CsvController.php to reflect the one i sent on #2939688: Fix the errors/warnings reported by PHP_CodeSniffer;

Also, 2939688 includes a bunch of other corrected DIs.

Needs review :)

I'll review it.

I've just ran all tests and looked at the site. No errors found.

Everything is working fine as it should.

I'm moving it to RTBC.

I made some small to the patch from #28 changes and committed that. I see that the changes only affected test modules so as long the tests still pass, it should be good. The changes do not impact the usage of the module.