Convert aggregator admin form to configuration system

Adding tags

The patch still applies. Fixing the test failures doesn't look like a novice task to me.

Reviewing it: Maybe the update hook should be in the system module, in case the module is disabled (which it very well can be)?

It is probably worth postponing this one until #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML lands, as that will clean up the config file significantly and probably simplify a few things.

This needs to be modified so that names match the new naming conventions laid out at http://drupal.org/node/1667896

Updated agains HEAD
* new naming conventions
* included system_config_form().

#16: 1496654_16_aggregator_cmi.patch queued for re-testing.

Thanks for working on this!

1. The default config file for aggregator is missing in the patch.
2. Let's name that config object aggregator.settings (instead of ".admin"), please. The "admin" part of the function does not have any special meaning.
3. Let's also make sure that the total set of settings makes sense when looking at the config object only. Based on how the values are presented in the settings form and how they're used, I suspect that there is room for grouping some of the keys into sub-keys.

   For example:

   fetcher: aggregator
   parser: aggregator
   processors:
       - aggregator
   items:
       allowed_html: ...
       teaser_length: '600'
       expire: '9676800'
   source:
       list_max: '3'
       category_selector: checkboxes
   

   Note: clear is renamed to items.expire, and summary_items renamed to source.list_max in the above.

   Of course, this is just a suggestion; feel free to improve the keys and sub-keys where you see fit.

Furthermore:

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -484,7 +484,7 @@ function aggregator_admin_form($form, $form_state) {
+      '#default_value' => config('aggregator.admin')->get('fetchter'),

Typo in 'fetchter'.

+++ b/core/modules/aggregator/aggregator.install
@@ -9,14 +9,8 @@
  * Implements hook_uninstall().
  */
 function aggregator_uninstall() {
-  variable_del('aggregator_allowed_html_tags');
-  variable_del('aggregator_summary_items');
-  variable_del('aggregator_clear');
-  variable_del('aggregator_category_selector');
-  variable_del('aggregator_fetcher');
-  variable_del('aggregator_parser');
-  variable_del('aggregator_processors');
-  variable_del('aggregator_teaser_length');
+  config('aggregator.admin')
+    ->delete();
 }

aggregator_uninstall() can be removed entirely. All configuration objects of a module are deleted automatically upon uninstallation, based on their primary namespace (i.e., aggregator.).

+++ b/core/modules/aggregator/aggregator.install
@@ -271,3 +265,10 @@ function aggregator_schema() {
+/**
+  * Moves aggregator settings from variables to config.
+  */
+function aggregrator_update_8000() {
+  update_variables_to_config('aggregator.admin');

1) update_variables_to_config() needs an explicit mapping from old variable names to new config object key names.

2) Bogus phpDoc comment formatting.

Oh thx sun ;-)

On my local machine the test case testAtomSample was not completely successfully. lets see what testbot say.

Oh thats was the mistake. just a save ;-) Now the test works. But I#m wondering me why I dont see anything about db-updates for aggregator_update_8000(). Is there also something new?

#24: 1496654_24_aggregator_cmi.patch queued for re-testing.

YAY, thanks folks! :)

I like this patch, and committed it to 8.x.

Personally, I think the chaining is a unconventional in this case, as it seems completely unnecessary. Not a deal-breaker though.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -519,23 +519,31 @@ function aggregator_admin_form($form, $form_state) {
+  $config = config('aggregator.settings');
+  $config
+    ->set('items.allowed_html', $form_state['values']['aggregator_allowed_html_tags'])
+    ->set('items.expire', $form_state['values']['aggregator_clear'])
+    ->set('items.teaser_length', $form_state['values']['aggregator_teaser_length'])
+    ->set('source.list_max', $form_state['values']['aggregator_summary_items'])

I think @Dries meant to mark this fixed.

Also, these conversion issues should be tagged with API change, since they technically are API changes. (Doing so also sends announcements to @drupalchanges.)

Head is broken because defaults for aggregator are missing. Looks like the CMI file added by this patch wasn't pushed up with the commit.

The CMI file is now added to 8.x. Head may still be broken though...

Nope, all good again, thanks!

Don't we need

config_install_default_config('aggregator')?

never mind

Sun quote from another cmi issue

Additionally, update_variables_to_config() "installs" (merges) the default configuration of a module already (if appropriate), so this call to config_install_default_config() should be removed.

wrong update function name:P
#1958702: Aggregator variables to config update is never executed

Added a link to the summary issue