Part of #1696224: Remove system_settings_form()

The aggregator settings at admin/config/services/aggregator/settings need to be converted to use the new configuration system.

Tasks

Create a default config file and add it to the aggregator module.
Convert the admin UI in aggregator.admin.inc to read to/write from the appropriate config.
Convert any code that currently accesses the variables used to use the config system.
Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.

Files: 
CommentFileSizeAuthor
#24 1496654_24_aggregator_cmi.patch15.88 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 37,103 pass(es). View
#23 1496654_23_aggregator_cmi.patch15.88 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 37,107 pass(es). View
#21 1496654_20_aggregator_cmi.patch15.88 KBtobiasb
FAILED: [[SimpleTest]]: [MySQL] 37,106 pass(es), 4 fail(s), and 0 exception(s). View
#16 1496654_16_aggregator_cmi.patch14.74 KBtobiasb
FAILED: [[SimpleTest]]: [MySQL] 36,992 pass(es), 62 fail(s), and 37 exception(s). View
#14 1496654_14_aggregator_cmi.patch14.75 KBtobiasb
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1496654_14_aggregator_cmi.patch. View
#12 aggreator_10_12_interdiff.txt342 bytesalexpott
#12 1496654_12_aggregator_cmi.patch15.31 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 36,667 pass(es). View
#10 1496654_10_aggregator_cmi.patch15.33 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 36,657 pass(es), 4 fail(s), and 0 exception(s). View
#10 aggreator_8_10_interdiff.txt5.56 KBalexpott
#8 1496654_8_aggregator_cmi.patch15.41 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 36,647 pass(es), 4 fail(s), and 0 exception(s). View
#2 1496654_aggregator_configuration_management.patch15.39 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 35,687 pass(es), 4 fail(s), and 0 exception(s). View

Comments

pcambra’s picture

Issue tags: +Config novice

Adding tags

alexpott’s picture

FileSize
15.39 KB
FAILED: [[SimpleTest]]: [MySQL] 35,687 pass(es), 4 fail(s), and 0 exception(s). View

First stab at converting aggregator module to use CMI.

Issues remaining:

<aggregator_allowed_html_tags>&lt;a&gt; &lt;b&gt; &lt;br&gt; &lt;dd&gt; &lt;dl&gt; &lt;dt&gt; &lt;em&gt; &lt;i&gt; &lt;li&gt; &lt;ol&gt; &lt;p&gt; &lt;strong&gt; &lt;u&gt; &lt;ul&gt;</aggregator_allowed_html_tags>

I think this should be (if we continue to use xml

<aggregator_allowed_html_tags><![CDATA[<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>]]></aggregator_allowed_html_tags>
alexpott’s picture

Status: Active » Needs review

Bump to needs review...

Status: Needs review » Needs work

The last submitted patch, 1496654_aggregator_configuration_management.patch, failed testing.

alexpott’s picture

Issue tags: +Configuration system

Fixing tags

Niklas Fiekas’s picture

Issue tags: -Config novice

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)?

heyrocker’s picture

Status: Needs work » Postponed

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.

alexpott’s picture

Status: Postponed » Needs review
FileSize
15.41 KB
FAILED: [[SimpleTest]]: [MySQL] 36,647 pass(es), 4 fail(s), and 0 exception(s). View

Now that Yaml has landed here's an updated patch

Status: Needs review » Needs work

The last submitted patch, 1496654_8_aggregator_cmi.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.56 KB
15.33 KB
FAILED: [[SimpleTest]]: [MySQL] 36,657 pass(es), 4 fail(s), and 0 exception(s). View

And now for a patch that passes all the tests and has some nice tidy ups :)

Status: Needs review » Needs work

The last submitted patch, 1496654_10_aggregator_cmi.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
15.31 KB
PASSED: [[SimpleTest]]: [MySQL] 36,667 pass(es). View
342 bytes

Borken uninstall :(

heyrocker’s picture

Status: Needs review » Needs work

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

tobiasb’s picture

Status: Needs work » Needs review
FileSize
14.75 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1496654_14_aggregator_cmi.patch. View

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

Status: Needs review » Needs work

The last submitted patch, 1496654_14_aggregator_cmi.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
14.74 KB
FAILED: [[SimpleTest]]: [MySQL] 36,992 pass(es), 62 fail(s), and 37 exception(s). View

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1496654_16_aggregator_cmi.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1496654_16_aggregator_cmi.patch, failed testing.

sun’s picture

Issue tags: +Config novice

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.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
15.88 KB
FAILED: [[SimpleTest]]: [MySQL] 37,106 pass(es), 4 fail(s), and 0 exception(s). View

Oh thx sun ;-)

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

Status: Needs review » Needs work

The last submitted patch, 1496654_20_aggregator_cmi.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Remove duplicate title

alexpott’s picture

Status: Needs work » Needs review
FileSize
15.88 KB
PASSED: [[SimpleTest]]: [MySQL] 37,107 pass(es). View

Fixed failing testby making the following change to #21:

FROM:

config('aggregator.settings')->set('items.expire', AGGREGATOR_CLEAR_NEVER);

TO:

config('aggregator.settings')->set('items.expire', AGGREGATOR_CLEAR_NEVER)->save();
alexpott’s picture

FileSize
15.88 KB
PASSED: [[SimpleTest]]: [MySQL] 37,103 pass(es). View

re sun's #20 - @tobiasb good work on doing all of this... my tidy ups have been really minor

1. Fixed
2. Done
3. Changed as suggested
4. update_variables_to_config map provided
5. The patch attached makes a final clean up of aggregrator_update_8000() of phpDoc comment formatting by removing a line.

tobiasb’s picture

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?

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, 1496654_24_aggregator_cmi.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Config novice

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

YAY, thanks folks! :)

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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'])
sun’s picture

Status: Needs work » Fixed
Issue tags: +API change

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

neclimdul’s picture

Title: Convert aggregator admin form to configuration system » [HEAD BROKEN] Convert aggregator admin form to configuration system
Priority: Normal » Critical
Status: Fixed » Needs work

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.

jhodgdon’s picture

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

Berdir’s picture

Title: [HEAD BROKEN] Convert aggregator admin form to configuration system » Convert aggregator admin form to configuration system
Priority: Critical » Normal
Status: Needs work » Fixed

Nope, all good again, thanks!

aspilicious’s picture

Don't we need

config_install_default_config('aggregator')?

aspilicious’s picture

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.

Status: Fixed » Closed (fixed)

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

ParisLiakos’s picture

ParisLiakos’s picture

Issue summary: View changes

Added a link to the summary issue