The RSS publishing settings at admin/config/services/rss-publishing need to be converted to use the new configuration system.

Tasks

  • Create a default config file and add it to the system module.
  • Convert the admin UI in system.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.

This would be a good task for someone wanting to get up to speed on how the new config system works. Feel free to ping me on IRC if you need help.

CommentFileSizeAuthor
#66 drupal8.config-rss.66.patch10.77 KBsun
#66 interdiff.txt256 bytessun
#65 drupal8.config-rss.65.patch11.72 KBaspilicious
#63 drupal8.config-rss.63.patch10.47 KBaspilicious
#57 drupal8.config-rss.57.patch10.47 KBsun
#55 drupal8.config-rss.55.patch10.48 KBsun
#53 drupal8.config-rss.53.patch10.77 KBsun
#49 1496462-config-rss-publishing-49.patch9.04 KBNiklas Fiekas
#48 1496462-config-rss-publishing-48.patch9.15 KBNiklas Fiekas
#48 1496462-config-rss-publishing-interdiff.txt434 bytesNiklas Fiekas
#43 1496462_43_agg_cmi.patch9.07 KBcosmicdreams
#41 1496462_41_agg_cmi.patch9.07 KBcosmicdreams
#32 drupal-convert_rss_settings_to_config_system-1496462-32.patch9.07 KBacrollet
#30 drupal-convert_rss_settings_to_config_system-1496462-30.patch9.07 KBacrollet
#26 drupal-convert_rss_settings_to_config_system-1496462-26.patch9.08 KBacrollet
#21 drupal-convert_rss_settings_to_config_system-1496462-21.patch9.99 KBacrollet
#20 drupal-convert_rss_settings_to_config_system-1496462-20.patch10.44 KBacrollet
#16 drupal-convert_rss_settings_to_config_system-1496462-16.patch10.53 KBacrollet
#11 drupal-convert_rss_settings_to_config_system-1496462-11.patch9.87 KBacrollet
#7 drupal-convert_rss_settings_to_config_system-1496462-7.patch9.59 KBacrollet
#4 drupal-convert_rss_settings_to_config_system-1496462-4.patch9.6 KBacrollet
#2 drupal-convert_rss_settings_to_config_system-1496462-2.patch9.6 KBacrollet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Assigned: Unassigned » acrollet

I'll work on this one.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Active » Needs review
FileSize
drupal-convert_rss_settings_to_config_system-1496462-2.patch9.6 KB

patch attached.

Status: Needs review » Needs work

The last submitted patch, drupal-convert_rss_settings_to_config_system-1496462-2.patch, failed testing.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
FileSize
drupal-convert_rss_settings_to_config_system-1496462-4.patch9.6 KB

updated patch attached

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review

setting status

Status: Needs review » Needs work

The last submitted patch, drupal-convert_rss_settings_to_config_system-1496462-4.patch, failed testing.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
FileSize
drupal-convert_rss_settings_to_config_system-1496462-7.patch9.59 KB

I'm failing at life :( Attaching another version of the patch with the syntax error actually fixed.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review

annnnd setting status.

Status: Needs review » Needs work

The last submitted patch, drupal-convert_rss_settings_to_config_system-1496462-7.patch, failed testing.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented 
+++ b/core/modules/system/system.admin.incundefined
@@ -1866,29 +1866,49 @@ function system_image_toolkit_settings() {
+function system_rss_feeds_settings_submit($form, &$form_state) {
+  // Set the RSS publishing parameters.
+  config('system.rss-publishing')
+    ->set('feed_description', $form_state['values']['feed_description']) ¶
+    ->save();
+  config('system.rss-publishing')
+    ->set('feed_default_items', $form_state['values']['feed_default_items']) ¶
+    ->save();
+  config('system.rss-publishing')
+    ->set('feed_item_length', $form_state['values']['feed_item_length']) ¶

In general, you should only chain config() when you're only using it once in a function. Otherwise you are instantiating a config object three times which causes some unnecessary overhead. In these cases I would do

$config = config('my.file');
// do stuff with $config
$config->save();

+++ b/core/modules/system/system.installundefined
@@ -1761,6 +1761,26 @@ function system_update_8005() {
+function system_update_8006() {
+  $var_names = array_keys(config('system.rss-publishing')->get());
+  if (!empty($var_names)) {
+    $query = db_select('variable', 'v')
+      ->fields('v')
+      ->condition('name', $var_names, 'IN');
+    $del = db_delete('variable')->condition('name', $var_names, 'IN');
+    $var_values = $query->execute()->fetchAllKeyed(0);
+    $del->execute();
+    $config = config('system.rss-publishing');
+    foreach($var_values as $name => $val) {
+      $config->set($name, $val);
+    }
+    $config->save();
+  }
+}

I wouldn't mind seeing a couple lines of comments here, took me a minute to figure it out. A nice technique though, I might update some other update code to use it (I had just been hardcoding in the keys.)

Thanks!

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review
FileSize
drupal-convert_rss_settings_to_config_system-1496462-11.patch9.87 KB

New patch attached - I can't take credit for the code in the update hook, as it was lifted directly from #1493098: Convert cron settings to configuration system - that said, I added a few comments.

Status: Needs review » Needs work

The last submitted patch, drupal-convert_rss_settings_to_config_system-1496462-11.patch, failed testing.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Needs work » Needs review

I was just reviewing this patch and the code in system_update_8006() made me wonder about a couple of things that may well have been answered elsewhere. My thoughts are:

1) What happens if the update never runs? Do we get a WSOD if no config variables defined in xml file?
2) To ensure we preserve data in the case of the process stopping mid stream, I think we should first write the config file and then delete the variables.
3) Do we need to confirm we can get desired values from that config file before deleting variable stored value(s)?

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Needs review » Needs work
acrollet’s picture

acrollet CreditAttribution: acrollet commented

1) What happens if the update never runs? Do we get a WSOD if no config variables defined in xml file?
2) To ensure we preserve data in the case of the process stopping mid stream, I think we should first write the config file and then delete the variables.
3) Do we need to confirm we can get desired values from that config file before deleting variable stored value(s)?

per heyrocker: 2) is a concern, but 1) and 3) are not - the next patch I post will account for 2).

acrollet’s picture

acrollet CreditAttribution: acrollet commented
FileSize
drupal-convert_rss_settings_to_config_system-1496462-16.patch10.53 KB

new patch attached addressing (hopefully) testbot errors and comment #13.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review

issue status.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work 
+  <feed_description></feed_description>

i don't think we should ship defaults that are empty like this. the config system will create this as needed on save.

+    /*
+     * Get any variables currently defined that match the new 
+     * setting names in the config file.
+     */

these comments should be '// ....' style as per coding standards.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

i don't think we should ship defaults that are empty like this. the config system will create this as needed on save.

But then if there is code that tries to access this data before its been created, won't it die? I think this is a use case we really need to support. One thing I was thinking that might work is that on get() we can do

if(is_array($data) && empty($data) {
  $data = (string) $data;
}

The question is, is there ever a use case where an empty array is a valid return value? I personally think that an empty string is much more sensible for blank tags than an empty array.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
FileSize
drupal-convert_rss_settings_to_config_system-1496462-20.patch10.44 KB

Patch attached for if beejeebus wins the argument :)

acrollet’s picture

acrollet CreditAttribution: acrollet commented
FileSize
drupal-convert_rss_settings_to_config_system-1496462-21.patch9.99 KB

Patch attached for if heyrocker wins the argument :)

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review

issue status.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs review » Needs work

I'm going to make an executive decision that I am right. acrollet has created a new issue at #1497088: Empty configuration values should return an empty string instead of an empty array to change it, and this patch is blocked until that resolves.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Postponed

This issue depends on the resolution of #1497088: Empty configuration values should return an empty string instead of an empty array.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Postponed » Needs work

This issue also depends on #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
FileSize
drupal-convert_rss_settings_to_config_system-1496462-26.patch9.08 KB

updated patch attached, depends on the #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review

#1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system is RTBC, setting this to needs review.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Needs review » Needs work

A couple of things with this patch:

1) In class AggregatorRenderingTestCase extends AggregatorTestCase, I think we should use a variable:
$config = config('system.rss-publishing');
so that we do not initiate two config instances during this test case.

2) In the xml file,

+<config>
+  <feed_description></feed_description>

do we really want an empty element feed_description? That struck me as odd and leads me to wonder how we will specify NULL values or empty strings in the config files. It also led me to wonder why this element needs to be specified if it is empty.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

"do we really want an empty element feed_description? That struck me as odd and leads me to wonder how we will specify NULL values or empty strings in the config files. It also led me to wonder why this element needs to be specified if it is empty."

I think it is important to explicitly declare all the data this module is bringing along with it. This would return an empty string, NULL values can not be represented in the config system (they are reserved to indicate errors.)

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review
FileSize
drupal-convert_rss_settings_to_config_system-1496462-30.patch9.07 KB

patch attached addressing item 1) in #28.

Status: Needs review » Needs work

The last submitted patch, drupal-convert_rss_settings_to_config_system-1496462-30.patch, failed testing.

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review
FileSize
drupal-convert_rss_settings_to_config_system-1496462-32.patch9.07 KB

arrgh, attaching patch without syntax error, thought I had fixed that.

jthorson’s picture

jthorson CreditAttribution: jthorson commented

Sorry, this patch ran into testbot problems ... Not actually green.

Results:

35634 Passes
30 fails
8 exceptions.

BareUpgradeTest 2 fails
FilledUpgradeTest 2 fails
FATAL UserAutocompleteTestCase: test runner returned a non-zero error code (255).

Same problem on multiple bots. Please do not requeue #26 or #32.

acrollet’s picture

acrollet CreditAttribution: acrollet commented

BareUpgradeTest 2 fails
FilledUpgradeTest 2 fails

Those two tests (along with all the aggregator tests) run fine when also applying #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system, so I'll wait for that to get committed to re-queue.

Status: Needs review » Needs work

The last submitted patch, drupal-convert_rss_settings_to_config_system-1496462-32.patch, failed testing.

jthorson’s picture

jthorson CreditAttribution: jthorson commented

update_variables_to_config() does not exist in this patch or the latest D8 core, but is called in system.install.
The run also appears to be attempting a require on entity.inc from the /includes directory, instead of /core/modules/entity.

Here's a dump of the errors triggered on testbot, just before it trips an xmlrpc 'Request is not well formed' error and starts over again. (Either that, or just when it starts ... not quite sure which.)

[Mon Mar 26 21:36:07 2012] [error] [client 10.20.0.126] PHP Fatal error:  Call to undefined function update_variables_to_config() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.install on line 1768
[Mon Mar 26 21:36:11 2012] [error] [client 10.20.0.126] PHP Fatal error:  Call to undefined function update_variables_to_config() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.install on line 1768
[Mon Mar 26 21:36:13 2012] [error] [client 10.20.0.126] PHP Fatal error:  Call to undefined function update_variables_to_config() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.install on line 1768
[Mon Mar 26 21:36:13 2012] [error] [client 10.20.0.126] PHP Fatal error:  require_once(): Failed opening required '/var/lib/drupaltestbot/sites/default/files/checkout/includes/entity.inc' (include_path='.:/usr/share/php:/usr/share/pear') in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2914
[Mon Mar 26 21:36:13 2012] [error] [client 10.20.0.126] PHP Fatal error:  require_once(): Failed opening required '/var/lib/drupaltestbot/sites/default/files/checkout/includes/entity.inc' (include_path='.:/usr/share/php:/usr/share/pear') in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2914
[Mon Mar 26 21:36:13 2012] [error] [client 10.20.0.126] PHP Fatal error:  require_once(): Failed opening required '/var/lib/drupaltestbot/sites/default/files/checkout/includes/entity.inc' (include_path='.:/usr/share/php:/usr/share/pear') in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2914
[Mon Mar 26 21:36:13 2012] [error] [client 10.20.0.126] PHP Fatal error:  require_once(): Failed opening required '/var/lib/drupaltestbot/sites/default/files/checkout/includes/entity.inc' (include_path='.:/usr/share/php:/usr/share/pear') in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2914
[Mon Mar 26 21:36:19 2012] [error] [client 10.20.0.126] PHP Fatal error:  Call to undefined function update_variables_to_config() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.install on line 1768
gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

Now that update_variables_to_config() has been committed, this should work, so im going to requeue it.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs work » Needs review
gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

#32: drupal-convert_rss_settings_to_config_system-1496462-32.patch queued for re-testing.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs review » Needs work

This needs a reroll because system module update functions have moved on. Otherwise its probably ready to go.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
Status: Needs work » Needs review
FileSize
1496462_41_agg_cmi.patch9.07 KB

rerolled

Status: Needs review » Needs work

The last submitted patch, 1496462_41_agg_cmi.patch, failed testing.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
FileSize
1496462_43_agg_cmi.patch9.07 KB

fixed the update function.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1496462_43_agg_cmi.patch, failed testing.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

Current patch doesn't apply because of system update method numbering, rolling a new one and looking into the remaining test fails.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Assigned: acrollet » Niklas Fiekas

Looking.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Assigned: Niklas Fiekas » Unassigned
Status: Needs work » Needs review
FileSize
1496462-config-rss-publishing-interdiff.txt434 bytes
1496462-config-rss-publishing-48.patch9.15 KB

Rerolled for the update function. Curious if any test failures remain.
Edit: Wait ... the automerge did play a trick on me. I didn't intend to reorder the update hooks.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
FileSize
1496462-config-rss-publishing-49.patch9.04 KB

Next try.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs review » Reviewed & tested by the community

I think we are ready to go here, although there are currently at least three patches I've seen in the last hour with system_update_8010() defined so we will see who gets in first.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Great update, and glad to see more CMI-ification. Committed to 8.x.

Status: Fixed » Closed (fixed)

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: configuration system » system.module
Status: Closed (fixed) » Needs review
FileSize
drupal8.config-rss.53.patch10.77 KB

Follow-up patch to adjust the new conversion for
#1599554: Tutorial/guidelines for how to convert variables into configuration

Status: Needs review » Needs work

The last submitted patch, drupal8.config-rss.53.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.config-rss.55.patch10.48 KB

Sorry. Re-rolled against HEAD.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-rss.55.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.config-rss.57.patch10.47 KB

Wow, I'm really sorry. This one should pass.

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

The last submitted patch, drupal8.config-rss.57.patch, failed testing.

ZenDoodles’s picture

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

#57: drupal8.config-rss.57.patch queued for re-testing.

ZenDoodles’s picture

ZenDoodles CreditAttribution: ZenDoodles commented

Retesting. Error seems unrelated to the patch:

Fatal error: Call to undefined function Drupal\system\Tests\Upgrade\language_negotiation_include() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.php on line 144
FATAL Drupal\system\Tests\Upgrade\LanguageUpgradePathTest: test runner returned a non-zero error code (255).
- Found database prefix 'simpletest889334' for test ID 456.
[03-Jul-2012 16:52:44] PHP Fatal error:  Call to undefined function Drupal\system\Tests\Upgrade\language_negotiation_include() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.php on line 144

Status: Needs review » Needs work

The last submitted patch, drupal8.config-rss.57.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +API change

All config system conversions are API changes, so tagging as such.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
drupal8.config-rss.63.patch10.47 KB

re-uploading

Status: Needs review » Needs work

The last submitted patch, drupal8.config-rss.63.patch, failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
drupal8.config-rss.65.patch11.72 KB

Ok so if you forget to add the rss config file, language module doesn't get enabled on upgrade. It feels like the environment doesn't get cleaned up correctly after a fatal error.

Anyway, this should be better.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff.txt256 bytes
drupal8.config-rss.66.patch10.77 KB

Thanks!

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

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