Make possible for parsers and fetchers to expose configuration through plugins

Why this postponed?

While there's no code no collisions. Also I see no dependency between

This should definitely use FormInterface. What's wrong with getFormID()?

The title of this issue implies that exposing configuration should be possible, but not required, I would say checking if ($parser instanceof FormInterface) { $form = drupal_get_form($parser); } is your best bet.

The patch looks pretty good. I haven't done a fine-grained pass on it yet.

rootatwc and I discussed this on IRC. I had mentioned that we might like to separate the form classes from the plugins. Now I'm thinking that's probably a good idea for 2 reasons.

1. if bundles ever get anywhere (this can also be done in contrib I'm pretty sure), then each plugin might want to expose 2 forms. One for the aggregator type and one for the feed item.
2. it makes forms much more easily re-used. Allowing a common URL entry field, or a very complex configuration form to be re-used without having to inherit from an other-wise unrelated class.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.phpundefined
@@ -18,6 +20,13 @@
+   * The configuration object factory.
+   *
+   * @var \Drupal\Core\Config\ConfigFactory
+   */
+  protected $configFactory;
+
+  /**

+1 to this change

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.phpundefined
@@ -36,6 +45,13 @@ class SettingsForm extends SystemConfigFormBase {
+   * The instanciated plugin instances.

*instantiated

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -28,11 +30,30 @@
-   * Implements \Drupal\aggregator\Plugin\FetcherInterface::fetch().
+   * The HTTP client to fetch the feed data with.
+   *
+   * @var \Guzzle\Http\Client
+  */

Asterisk is off.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -28,11 +30,30 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Client $http_client) {
+    $this->httpClient = $http_client;
+  }
+  /**
+   * {@inheritdoc}

Missing new line.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.phpundefined
@@ -84,9 +108,9 @@ public function settingsForm(array $form, array &$form_state) {
-  public function settingsSubmit(array $form, array &$form_state) {
+  public function submitForm(array &$form, array &$form_state) {
     $config = config('aggregator.settings');
     $config->set('items.expire', $form_state['values']['aggregator_clear'])
       ->set('items.teaser_length', $form_state['values']['aggregator_teaser_length'])

Should be using $this->configFactory.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.phpundefined
@@ -147,7 +171,7 @@ public function process(Feed $feed) {
   public function remove(Feed $feed) {
     $iids = Database::getConnection()->query('SELECT iid FROM {aggregator_item} WHERE fid = :fid', array(':fid' => $feed->id()))->fetchCol();

Should we inject the Database and Entity manager/storage controller?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.phpundefined
@@ -159,9 +183,7 @@ public function remove(Feed $feed) {
   public function postProcess(Feed $feed) {
     $aggregator_clear = config('aggregator.settings')->get('items.expire');
diff --git a/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.php b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.php

$this->configFactory

+++ b/core/modules/aggregator/tests/Drupal/aggregator/Tests/Plugin/AggregatorPluginSettingsBaseTest.phpundefined
@@ -0,0 +1,103 @@
+    $test_processor = $this->getMock(
+      'Drupal\aggregator_test\Plugin\aggregator\processor\TestProcessor',
+      array('buildForm', 'validateForm', 'submitForm'),
+      array(array(), 'aggregator_test', array(), $this->configFactory)

Awesome!

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/AggregatorPluginSettingsBase.phpundefined
@@ -0,0 +1,44 @@
+   * {@inheritdoc}
+   */
+  public function validateForm(array &$form, array &$form_state) {
...
+   * {@inheritdoc}
+   */
+  public function submitForm(array &$form, array &$form_state) {

not sure it's fine to have empty implementations

It's a matter of having possibly several empty methods vs. one. It's a pretty common pattern, not sure about core.

Having an empty validate is fine, but there is no point of a form without a submit, that shouldn't be in the base class so subclasses are forced to implement

Patch looks great! except:

+++ b/core/modules/aggregator/tests/Drupal/aggregator/Tests/Plugin/AggregatorPluginSettingsBaseTest.phpundefined
@@ -0,0 +1,119 @@
+// @todo Remove when drupal_set_message() is injectable.
+namespace {
+  if (!function_exists('drupal_set_message')) {
+    function drupal_set_message() {}

WTF?

Removing drupal_set_title() is AFAIK still a release-blocking critical to actually get the benefits of all the new stuff we put into core. Maybe add a @todo and say remove when we fix issue X (X being the critical where this is being talked about). I think that would help to actually remove it when doing so.

Looks like ready to go! @twistor RTBC?

So close!

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.phpundefined
@@ -47,8 +66,9 @@ class SettingsForm extends SystemConfigFormBase {
+    $this->translation = $string_translation;

$this->translation needs to be defined as a property.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/AggregatorPluginManager.phpundefined
@@ -38,8 +40,8 @@ public function __construct($type, \Traversable $namespaces) {
+    parent::__construct("aggregator/$type", $namespaces, $annotation_namespaces, $type_annotations[$type]);
+    $this->setCacheBackend($cache_backend, $language_manager, "aggregator_$type");

Should we call alterInfo() here?

Altering could be added as api-addition after july 1

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.phpundefined
@@ -129,35 +157,71 @@ public function buildForm(array $form, array &$form_state) {
-    return parent::buildForm($form, $form_state);
+
+    // @todo remove in https://drupal.org/node/2018411 or after its in.
+    $form['actions']['#type'] = 'actions';
+    $form['actions']['submit'] = array(
+      '#type' => 'submit',
+      '#value' => $this->translation->translate('Save configuration'),
+      '#button_type' => 'primary',
+    );
+    $form['#theme'] = 'system_config_form';
+    return $form;

Doing this just to phpunit test functionality is not right... now if anything changes in the parent::buildForm() then we need to remember to change it here... if we want to test this with phpUnit we have to postpone on #2018411: Figure out a nice DX when working with injected translation

+++ b/core/modules/aggregator/tests/Drupal/aggregator/Tests/Plugin/AggregatorPluginSettingsBaseTest.phpundefined
@@ -0,0 +1,119 @@
+// @todo Remove after https://drupal.org/node/1830588 is in.

Wrong link this is for drupal_set_title :)

Some tests currently just define a

function t($string) {
  return $string;
}

at the top of their test to avoid that problem. Don't know if we want to do that here...

Yes, that works fine for constants, but for functions, you get into trouble due to namespaces, and need to do ugly things with multiple namespaces in the same file.

Awesome!

Needs a reroll

curl https://drupal.org/files/drupal-aggregator_plugins_settings-1957330-51.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27040  100 27040    0     0   7498      0  0:00:03  0:00:03 --:--:--  7971
error: patch failed: core/modules/aggregator/aggregator.services.yml:1
error: core/modules/aggregator/aggregator.services.yml: patch does not apply
error: patch failed: core/modules/aggregator/lib/Drupal/aggregator/Plugin/AggregatorPluginManager.php:38
error: core/modules/aggregator/lib/Drupal/aggregator/Plugin/AggregatorPluginManager.php: patch does not apply

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -7,12 +7,15 @@
+use Guzzle\Http\Client;

@@ -25,14 +28,37 @@
+  public function __construct(Client $http_client) {

We should typehint to the interface considering we have one.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.phpundefined
@@ -136,28 +153,56 @@ public function buildForm(array $form, array &$form_state) {
+          $this->instances[] = $instance;
...
+          $this->instances[] = $instance;

Shouldn't you key the instances so we don't have dupes? Though the three plugin types could have clashes.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.phpundefined
@@ -136,28 +153,56 @@ public function buildForm(array $form, array &$form_state) {
+    foreach ($this->instances as $instance) {

Maybe rename $this->instances to indicate these are ones that implement FormInterface...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/AggregatorPluginSettingsBase.phpundefined
@@ -0,0 +1,31 @@
+  public function getFormID() {
+    return 'aggregator_admin_form';

This just feels like an abuse of FormInterface, actually.
In every other case we don't reuse the interface, we just have another interface like \Drupal\Core\Action\ConfigurableActionInterface, until #2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects is resolved...

Actually the interfaces added in #2033383: Provide a default plugin bag are very relevant here:
PluginFormInterface
ConfigurablePluginInterface

I'll take a stab at this.

Still needs some work, want to see what the testbot says.

Bah

Not sure about the changes to AggregatorPluginSettingsBaseTest still wrapping my head around that stuff.

#67: drupal-aggregator_plugins_settings-1957330-67.patch queued for re-testing.

This patch looks awesome! Only minor things, it's essentially RTBC

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php
   @@ -136,28 +153,56 @@ public function buildForm(array $form, array &$form_state) {
   +          $this->configurableInstances[] = $instance;
   ...
   +          $this->configurableInstances[] = $instance;
   

   Any benefit to keying this by $id?

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
   @@ -25,14 +28,37 @@
   +    return new static($container->get('http_default_client'));
   
   +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php
   @@ -25,14 +28,44 @@
   +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('config.factory'));
   
   +++ b/core/modules/aggregator/tests/modules/aggregator_test/lib/Drupal/aggregator_test/Plugin/aggregator/processor/TestProcessor.php
   @@ -24,14 +27,44 @@
   +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('config.factory'));
   

   Can these be split onto multiple lines please?

3. +++ b/core/modules/aggregator/tests/modules/aggregator_test/lib/Drupal/aggregator_test/Plugin/aggregator/processor/TestProcessor.php
   @@ -24,14 +27,44 @@
   +    parent::__construct($configuration + $this->getConfiguration(), $plugin_id, $plugin_definition);
   
   @@ -46,22 +79,21 @@ public function settingsForm(array $form, array &$form_state) {
   +  public function submitConfigurationForm(array &$form, array &$form_state) {
   ...
   +    $this->setConfiguration($this->configuration);
   

   Since this is the first implementation of ConfigurablePluginInterface and PluginFormInterface to not be managed by a PluginBag/ConfigEntity, this is a little weird to call this here. Let's add an @todo in case we ever refactor for ConfigEntity.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php
@@ -161,6 +161,8 @@ public function buildForm(array $form, array &$form_state) {
+          // Keying by id would bring conflicts, because two instances of a

Here and elsewhere, s/id/ID.

Also, #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity went in, which will break the patch.

Thanks, looks RTBC (again) to me!

Needs a reroll

git ac https://drupal.org/files/drupal-aggregator_plugins_settings-1957330-78.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32573  100 32573    0     0  21692      0  0:00:01  0:00:01 --:--:-- 24657
error: patch failed: core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php:7
error: core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php: patch does not apply

And back to RTBC as per #80.

Committed bc602a7 and pushed to 8.x. Thanks!

This one we blame on d.o.