Right now, only processors can expose configuration from the plugins..if you need to provide configuration for a parser or fetcher you have to hoook_form_alter
We should make the API consistent and provide this ability for parsers and fetchers as well

Postponed on
#1925048: Convert aggregator's system_config_form() to SystemConfigFormBase

Files: 
CommentFileSizeAuthor
#84 drupal-aggregator_plugins_settings-1957330-84.patch37.22 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]
#84 interdiff.txt8.07 KBParisLiakos
#82 drupal-aggregator_plugins_settings-1957330-82.patch31.82 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,573 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#78 drupal-aggregator_plugins_settings-1957330-78.patch31.81 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,828 pass(es).
[ View ]
#78 interdiff.txt1.52 KBParisLiakos
#75 drupal-aggregator_plugins_settings-1957330-75.patch31.86 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_plugins_settings-1957330-75.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#75 interdiff.txt5.06 KBParisLiakos
#73 interdiff.txt1.42 KBParisLiakos
#73 drupal-aggregator_plugins_settings-1957330-73.patch31.33 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,040 pass(es).
[ View ]
#72 drupal-aggregator_plugins_settings-1957330-72.patch31.8 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#72 interdiff.txt9.24 KBParisLiakos
#71 drupal-aggregator_plugins_settings-1957330-71.patch28.3 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,072 pass(es).
[ View ]
#67 drupal-aggregator_plugins_settings-1957330-67.patch28.13 KBtwistor
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#65 drupal-aggregator_plugins_settings-1957330-65.patch27.65 KBtwistor
FAILED: [[SimpleTest]]: [MySQL] 58,026 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#63 drupal-aggregator_plugins_settings-1957330-63.patch27.6 KBtwistor
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#58 drupal-aggregator_plugins_settings-1957330-58.patch26.56 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,824 pass(es).
[ View ]
#58 interdiff.txt1.39 KBParisLiakos
#54 drupal-aggregator_plugins_settings-1957330-54.patch26.53 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,272 pass(es).
[ View ]
#51 drupal-aggregator_plugins_settings-1957330-51.patch26.41 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,986 pass(es).
[ View ]
#51 interdiff.txt8.34 KBParisLiakos
#46 drupal-aggregator_plugins_settings-1957330-46.patch33.02 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,000 pass(es).
[ View ]
#46 interdiff.txt652 bytesParisLiakos
#43 drupal-aggregator_plugins_settings-1957330-43.patch32.87 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]
#43 interdiff.txt733 bytesParisLiakos
#39 drupal-aggregator_plugins_settings-1957330-39.patch32.87 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,439 pass(es).
[ View ]
#36 drupal-aggregator_plugins_settings-1957330-36.patch35.66 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,337 pass(es).
[ View ]
#36 interdiff.txt5.78 KBParisLiakos
#33 drupal-aggregator_plugins_settings-1957330-33.patch35.83 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,768 pass(es), 104 fail(s), and 26 exception(s).
[ View ]
#31 drupal-aggregator_plugins_settings-1957330-31.patch35.83 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_plugins_settings-1957330-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 drupal-aggregator_plugins_settings-1957330-30.patch21.38 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#30 interdiff.txt1.42 KBParisLiakos
#29 drupal-aggregator_plugins_settings-1957330-29.patch35.79 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#28 drupal-aggregator_plugins_settings-1957330-28.patch30.06 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,360 pass(es).
[ View ]
#27 drupal-aggregator_plugins_settings-1957330-27.patch30.05 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,229 pass(es).
[ View ]
#26 interdiff.txt793 bytesParisLiakos
#25 drupal-aggregator_plugins_settings-1957330-25.patch30.55 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,745 pass(es).
[ View ]
#19 drupal-aggregator_plugins_settings-1957330-19.patch30.78 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,487 pass(es).
[ View ]
#19 interdiff.txt2.77 KBParisLiakos
#17 drupal-aggregator_plugins_settings-1957330-17.patch30.41 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,474 pass(es).
[ View ]
#17 interdiff.txt6.84 KBParisLiakos
#15 drupal-aggregator_plugins_settings-1957330-15.patch28.33 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 54,380 pass(es), 69 fail(s), and 59 exception(s).
[ View ]
#13 drupal-aggregator_plugins_settings-1957330-13.patch14.76 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,453 pass(es).
[ View ]
#13 interdiff.txt4.92 KBParisLiakos
#11 drupal-aggregator_plugins_settings-1957330-11.patch16.35 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,496 pass(es).
[ View ]
#9 drupal-aggregator_plugins_settigns-1957330-8.patch15.2 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,472 pass(es).
[ View ]
#9 interdiff.txt3.63 KBParisLiakos
#7 drupal-aggregator_plugins_settigns-1957330-7.patch13.63 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,568 pass(es).
[ View ]
#7 interdiff.txt2.45 KBParisLiakos
#5 drupal-aggregator_plugins_settigns-1957330-5.patch12.88 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,368 pass(es).
[ View ]

Comments

andypost’s picture

Why this postponed?

ParisLiakos’s picture

Because they touch the same code and the other patch is ready.no point starting this one and then reroll it..you think we should re-postpone the other issue?

andypost’s picture

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

ParisLiakos’s picture

Assigned:Unassigned» ParisLiakos

having a go at this we definitely need this cleanup soon

ParisLiakos’s picture

Status:Active» Needs review
StatusFileSize
new12.88 KB
PASSED: [[SimpleTest]]: [MySQL] 54,368 pass(es).
[ View ]

what about this?

ParisLiakos’s picture

Issue tags:+Needs tests
StatusFileSize
new2.45 KB
new13.63 KB
PASSED: [[SimpleTest]]: [MySQL] 54,568 pass(es).
[ View ]

Added validate..i thought extending FormInterface, but it has getFormId() :/

tim.plunkett’s picture

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

ParisLiakos’s picture

StatusFileSize
new3.63 KB
new15.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,472 pass(es).
[ View ]

and we have to actually call it:)

ParisLiakos’s picture

@timplunkett you are right..i thought about this in a completely different way
patch coming

ParisLiakos’s picture

StatusFileSize
new16.35 KB
PASSED: [[SimpleTest]]: [MySQL] 54,496 pass(es).
[ View ]

here is it..
i am not sure if extending fetcher, parser processor interfaces with formInterface is ok, i just want to save checking if the plugin has this method in admin form..right now we are sure that a plugin has those methods..but maybe i should not extend them and just check in admin form whether the plugin extends FormInterface before calling the method?

tim.plunkett’s picture

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.

ParisLiakos’s picture

StatusFileSize
new4.92 KB
new14.76 KB
PASSED: [[SimpleTest]]: [MySQL] 54,453 pass(es).
[ View ]

agreed..also think its better for DX..if you want your plugin to expose settings extend AggregatorPluginSettingsBase

twistor’s picture

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.
ParisLiakos’s picture

Issue tags:-Needs tests
StatusFileSize
new28.33 KB
FAILED: [[SimpleTest]]: [MySQL] 54,380 pass(es), 69 fail(s), and 59 exception(s).
[ View ]

Here is a unit test..it contains patch from
#1974266: Register test module classes to PHPunit
i also moved aggregator_test modules in a modules folder

larowlan’s picture

+++ 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

ParisLiakos’s picture

StatusFileSize
new6.84 KB
new30.41 KB
PASSED: [[SimpleTest]]: [MySQL] 54,474 pass(es).
[ View ]

Yeah, had to change the references to the files that moved..also i missed the @ from inheritdocs:P

twistor’s picture

Status:Needs review» Needs work
+++ 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!

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new2.77 KB
new30.78 KB
PASSED: [[SimpleTest]]: [MySQL] 54,487 pass(es).
[ View ]

thanks for the review and nice catches!

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

Let's leave this for #1957312: Use the entity storage controller in aggregator module

andypost’s picture

+++ 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

ParisLiakos’s picture

i dont think an empty method in an abstract class is something bad.
\Drupal\system\SystemConfigFormBase::validateForm() does the same
that way, classes extending dont have to declare a validate method. not all forms needs validation

twistor’s picture

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

tim.plunkett’s picture

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

ParisLiakos’s picture

Agreed. But then this is the same for buildForm()..so i ll roll a patch later that removes those two methods

ParisLiakos’s picture

StatusFileSize
new30.55 KB
PASSED: [[SimpleTest]]: [MySQL] 54,745 pass(es).
[ View ]
ParisLiakos’s picture

StatusFileSize
new793 bytes

forgot interdiff:/

ParisLiakos’s picture

StatusFileSize
new30.05 KB
PASSED: [[SimpleTest]]: [MySQL] 55,229 pass(es).
[ View ]

#1974266: Register test module classes to PHPunit went in, this should be good to go
rerolling

ParisLiakos’s picture

StatusFileSize
new30.06 KB
PASSED: [[SimpleTest]]: [MySQL] 55,360 pass(es).
[ View ]

no longer applies, manually edited

ParisLiakos’s picture

StatusFileSize
new35.79 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
ParisLiakos’s picture

Issue tags:+phpunit
StatusFileSize
new1.42 KB
new21.38 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

sigh

ParisLiakos’s picture

StatusFileSize
new35.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_plugins_settings-1957330-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

one more fail...

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_plugins_settings-1957330-31.patch, failed testing.

ParisLiakos’s picture

StatusFileSize
new35.83 KB
FAILED: [[SimpleTest]]: [MySQL] 55,768 pass(es), 104 fail(s), and 26 exception(s).
[ View ]

reroll

ParisLiakos’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_plugins_settings-1957330-33.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new5.78 KB
new35.66 KB
PASSED: [[SimpleTest]]: [MySQL] 57,337 pass(es).
[ View ]

i totally messed up the reroll:)

ParisLiakos’s picture

ParisLiakos’s picture

Status:Needs review» Postponed
ParisLiakos’s picture

Status:Postponed» Needs review
StatusFileSize
new32.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,439 pass(es).
[ View ]

test module was moved in another issue.
AggregatorPluginManager now extends the shiny new DefaultPluginManager..
lets get this in, since it is an API change. i will fix the translations things after the DX issue is in

andypost’s picture

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?

ParisLiakos’s picture

heh, drupal_set_title kills the phpunit test, so i cant actually test submit form method..i used to have it in a comment, but then inspired from a views_ui test so i guessed its ok...we buy one more test, for some ugliness in a test class. i would rather have the test

Berdir’s picture

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.

ParisLiakos’s picture

StatusFileSize
new733 bytes
new32.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]

you are right, a link always help!

andypost’s picture

Looks like ready to go! @twistor RTBC?

twistor’s picture

Status:Needs review» Needs work

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?

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new652 bytes
new33.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,000 pass(es).
[ View ]

Should we call alterInfo() here?

Well there was not an alter hook before, i guess we would need some documentation if we add it?

i fix the property

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Altering could be added as api-addition after july 1

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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 :)

tstoeckler’s picture

Some tests currently just define a

<?php
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...

Berdir’s picture

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.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new8.34 KB
new26.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,986 pass(es).
[ View ]

Actually its a nice idea, lets do that, at least we wont have to hack out the parent call

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Awesome!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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
ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new26.53 KB
PASSED: [[SimpleTest]]: [MySQL] 56,272 pass(es).
[ View ]

conflicted with /aggregator/categories routing conversion

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

back to rtbc

YesCT’s picture

Issue tags:+RTBC July 1

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
new26.56 KB
PASSED: [[SimpleTest]]: [MySQL] 56,824 pass(es).
[ View ]

good point

tim.plunkett’s picture

+++ 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...

tim.plunkett’s picture

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

ParisLiakos’s picture

Assigned:ParisLiakos» Unassigned

not much time, unassigning in case someone wants to work on it

twistor’s picture

Assigned:Unassigned» twistor
Status:Needs review» Needs work

I'll take a stab at this.

twistor’s picture

Status:Needs work» Needs review
StatusFileSize
new27.6 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_plugins_settings-1957330-63.patch, failed testing.

twistor’s picture

Status:Needs work» Needs review
StatusFileSize
new27.65 KB
FAILED: [[SimpleTest]]: [MySQL] 58,026 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Bah

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_plugins_settings-1957330-65.patch, failed testing.

twistor’s picture

Status:Needs work» Needs review
StatusFileSize
new28.13 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

Status:Needs review» Needs work
Issue tags:-phpunit, -RTBC July 1

The last submitted patch, drupal-aggregator_plugins_settings-1957330-67.patch, failed testing.

twistor’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+phpunit, +RTBC July 1

The last submitted patch, drupal-aggregator_plugins_settings-1957330-67.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new28.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,072 pass(es).
[ View ]

thanks @twistor for working on this!
i rerolled it, no longer applies, and fixed the phpunit test

ParisLiakos’s picture

StatusFileSize
new9.24 KB
new31.8 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

so, lets make use of ConfigurablePluginInterface too

ParisLiakos’s picture

StatusFileSize
new31.33 KB
PASSED: [[SimpleTest]]: [MySQL] 58,040 pass(es).
[ View ]
new1.42 KB
+++ b/core/modules/aggregator/tests/modules/aggregator_test/lib/Drupal/aggregator_test/Plugin/aggregator/processor/TestProcessor.phpundefined
@@ -24,44 +27,73 @@
-    $form['processors'][$info['id']] = array(
+    $form['processors'][$this->getPluginId()] = array(
...
-      '#collapsed' => !in_array($info['id'], $processors),
+      '#collapsed' => !in_array($this->getPluginId(), $processors),
...
-    $form['processors'][$info['id']]['dummy_length'] = array(
+    $form['processors'][$this->getPluginId()]['dummy_length'] = array(

not sure how this jumped in

tim.plunkett’s picture

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.

ParisLiakos’s picture

StatusFileSize
new5.06 KB
new31.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-aggregator_plugins_settings-1957330-75.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

thanks for the review:)

this patch takes care of the points above

tim.plunkett’s picture

+++ 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.

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_plugins_settings-1957330-75.patch, failed testing.

ParisLiakos’s picture

StatusFileSize
new1.52 KB
new31.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,828 pass(es).
[ View ]

fixed ID and rerolled

ParisLiakos’s picture

Status:Needs work» Needs review
tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Thanks, looks RTBC (again) to me!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

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
ParisLiakos’s picture

Assigned:twistor» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new31.82 KB
FAILED: [[SimpleTest]]: [MySQL] 58,573 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

some use statements conflict with FormBase patch

Status:Needs review» Needs work

The last submitted patch, drupal-aggregator_plugins_settings-1957330-82.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new8.07 KB
new37.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]

Yay for FormBase

jibran’s picture

Status:Needs review» Reviewed & tested by the community

And back to RTBC as per #80.

alexpott’s picture

Title:Make possible for parsers and fetchers to expose configuration through plugins» Change notice: Make possible for parsers and fetchers to expose configuration through plugins
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed bc602a7 and pushed to 8.x. Thanks!

ParisLiakos’s picture

Title:Change notice: Make possible for parsers and fetchers to expose configuration through plugins» Make possible for parsers and fetchers to expose configuration through plugins
Priority:Critical» Normal
Status:Active» Fixed
Issue tags:-Needs change record

thanks everyone helping me out with this issue!

change notice is here https://drupal.org/node/2078169

Cheers

Status:Fixed» Closed (fixed)
Issue tags:+Needs change record

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags:-Needs change record

This one we blame on d.o.