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.

Files: 
CommentFileSizeAuthor
#66 drupal8.config-rss.66.patch10.77 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,108 pass(es). View
#66 interdiff.txt256 bytessun
#65 drupal8.config-rss.65.patch11.72 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 37,109 pass(es). View
#63 drupal8.config-rss.63.patch10.47 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#57 drupal8.config-rss.57.patch10.47 KBsun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#55 drupal8.config-rss.55.patch10.48 KBsun
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.admin.inc. View
#53 drupal8.config-rss.53.patch10.77 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.config-rss.53.patch. Unable to apply patch. See the log in the details link for more information. View
#49 1496462-config-rss-publishing-49.patch9.04 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 36,605 pass(es). View
#48 1496462-config-rss-publishing-48.patch9.15 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 36,263 pass(es), 5 fail(s), and 0 exception(s). View
#48 1496462-config-rss-publishing-interdiff.txt434 bytesNiklas Fiekas
#43 1496462_43_agg_cmi.patch9.07 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 35,572 pass(es), 1 fail(s), and 2 exception(s). View
#41 1496462_41_agg_cmi.patch9.07 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#32 drupal-convert_rss_settings_to_config_system-1496462-32.patch9.07 KBacrollet
PASSED: [[SimpleTest]]: [MySQL] 35,909 pass(es). View
#30 drupal-convert_rss_settings_to_config_system-1496462-30.patch9.07 KBacrollet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/aggregator/aggregator.test. View
#26 drupal-convert_rss_settings_to_config_system-1496462-26.patch9.08 KBacrollet
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View
#21 drupal-convert_rss_settings_to_config_system-1496462-21.patch9.99 KBacrollet
FAILED: [[SimpleTest]]: [MySQL] 35,693 pass(es), 0 fail(s), and 9 exception(s). View
#20 drupal-convert_rss_settings_to_config_system-1496462-20.patch10.44 KBacrollet
PASSED: [[SimpleTest]]: [MySQL] 35,693 pass(es). View
#16 drupal-convert_rss_settings_to_config_system-1496462-16.patch10.53 KBacrollet
PASSED: [[SimpleTest]]: [MySQL] 35,675 pass(es). View
#11 drupal-convert_rss_settings_to_config_system-1496462-11.patch9.87 KBacrollet
FAILED: [[SimpleTest]]: [MySQL] 35,681 pass(es), 0 fail(s), and 9 exception(s). View
#7 drupal-convert_rss_settings_to_config_system-1496462-7.patch9.59 KBacrollet
FAILED: [[SimpleTest]]: [MySQL] 35,680 pass(es), 0 fail(s), and 9 exception(s). View
#4 drupal-convert_rss_settings_to_config_system-1496462-4.patch9.6 KBacrollet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/aggregator/aggregator.pages.inc. View
#2 drupal-convert_rss_settings_to_config_system-1496462-2.patch9.6 KBacrollet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/aggregator/aggregator.pages.inc. View

Comments

acrollet’s picture

Assigned: Unassigned » acrollet

I'll work on this one.

acrollet’s picture

Status: Active » Needs review
FileSize
9.6 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/aggregator/aggregator.pages.inc. View

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

FileSize
9.6 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/aggregator/aggregator.pages.inc. View

updated patch attached

acrollet’s picture

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

FileSize
9.59 KB
FAILED: [[SimpleTest]]: [MySQL] 35,680 pass(es), 0 fail(s), and 9 exception(s). View

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

acrollet’s picture

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.

heyrocker’s picture

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

Status: Needs work » Needs review
FileSize
9.87 KB
FAILED: [[SimpleTest]]: [MySQL] 35,681 pass(es), 0 fail(s), and 9 exception(s). View

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

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

Status: Needs review » Needs work
acrollet’s picture

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

FileSize
10.53 KB
PASSED: [[SimpleTest]]: [MySQL] 35,675 pass(es). View

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

acrollet’s picture

Status: Needs work » Needs review

issue status.

beejeebus’s picture

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.

heyrocker’s picture

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

FileSize
10.44 KB
PASSED: [[SimpleTest]]: [MySQL] 35,693 pass(es). View

Patch attached for if beejeebus wins the argument :)

acrollet’s picture

FileSize
9.99 KB
FAILED: [[SimpleTest]]: [MySQL] 35,693 pass(es), 0 fail(s), and 9 exception(s). View

Patch attached for if heyrocker wins the argument :)

acrollet’s picture

Status: Needs work » Needs review

issue status.

heyrocker’s picture

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

Status: Needs work » Postponed
acrollet’s picture

acrollet’s picture

FileSize
9.08 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View
acrollet’s picture

Lars Toomre’s picture

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.

heyrocker’s picture

"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

Status: Needs work » Needs review
FileSize
9.07 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/aggregator/aggregator.test. View

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

Status: Needs work » Needs review
FileSize
9.07 KB
PASSED: [[SimpleTest]]: [MySQL] 35,909 pass(es). View

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

jthorson’s picture

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

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

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

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

heyrocker’s picture

Status: Needs work » Needs review
heyrocker’s picture

heyrocker’s picture

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

Status: Needs work » Needs review
FileSize
9.07 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View

rerolled

Status: Needs review » Needs work

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

cosmicdreams’s picture

FileSize
9.07 KB
FAILED: [[SimpleTest]]: [MySQL] 35,572 pass(es), 1 fail(s), and 2 exception(s). View

fixed the update function.

cosmicdreams’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

heyrocker’s picture

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

Assigned: acrollet » Niklas Fiekas

Looking.

Niklas Fiekas’s picture

Assigned: Niklas Fiekas » Unassigned
Status: Needs work » Needs review
FileSize
434 bytes
9.15 KB
FAILED: [[SimpleTest]]: [MySQL] 36,263 pass(es), 5 fail(s), and 0 exception(s). View

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

FileSize
9.04 KB
PASSED: [[SimpleTest]]: [MySQL] 36,605 pass(es). View

Next try.

heyrocker’s picture

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

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

Component: configuration system » system.module
Status: Closed (fixed) » Needs review
FileSize
10.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.config-rss.53.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
10.48 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.admin.inc. View

Sorry. Re-rolled against HEAD.

Status: Needs review » Needs work

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

sun’s picture

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

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

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

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

ZenDoodles’s picture

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

Issue tags: +API change

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

aspilicious’s picture

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

re-uploading

Status: Needs review » Needs work

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

aspilicious’s picture

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

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

Status: Needs review » Reviewed & tested by the community
FileSize
256 bytes
10.77 KB
PASSED: [[SimpleTest]]: [MySQL] 37,108 pass(es). View

Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

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