Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#66 | drupal8.config-rss.66.patch | 10.77 KB | sun |
#66 | interdiff.txt | 256 bytes | sun |
#65 | drupal8.config-rss.65.patch | 11.72 KB | aspilicious |
#63 | drupal8.config-rss.63.patch | 10.47 KB | aspilicious |
#57 | drupal8.config-rss.57.patch | 10.47 KB | sun |
Comments
Comment #1
acrollet CreditAttribution: acrollet commentedI'll work on this one.
Comment #2
acrollet CreditAttribution: acrollet commentedpatch attached.
Comment #4
acrollet CreditAttribution: acrollet commentedupdated patch attached
Comment #5
acrollet CreditAttribution: acrollet commentedsetting status
Comment #7
acrollet CreditAttribution: acrollet commentedI'm failing at life :( Attaching another version of the patch with the syntax error actually fixed.
Comment #8
acrollet CreditAttribution: acrollet commentedannnnd setting status.
Comment #10
gddIn 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
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!
Comment #11
acrollet CreditAttribution: acrollet commentedNew 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.
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commentedI 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)?
Comment #14
Lars Toomre CreditAttribution: Lars Toomre commentedComment #15
acrollet CreditAttribution: acrollet commentedper heyrocker: 2) is a concern, but 1) and 3) are not - the next patch I post will account for 2).
Comment #16
acrollet CreditAttribution: acrollet commentednew patch attached addressing (hopefully) testbot errors and comment #13.
Comment #17
acrollet CreditAttribution: acrollet commentedissue status.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedi don't think we should ship defaults that are empty like this. the config system will create this as needed on save.
these comments should be '// ....' style as per coding standards.
Comment #19
gddBut 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
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.
Comment #20
acrollet CreditAttribution: acrollet commentedPatch attached for if beejeebus wins the argument :)
Comment #21
acrollet CreditAttribution: acrollet commentedPatch attached for if heyrocker wins the argument :)
Comment #22
acrollet CreditAttribution: acrollet commentedissue status.
Comment #23
gddI'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.
Comment #24
acrollet CreditAttribution: acrollet commentedThis issue depends on the resolution of #1497088: Empty configuration values should return an empty string instead of an empty array.
Comment #25
acrollet CreditAttribution: acrollet commentedThis issue also depends on #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system.
Comment #26
acrollet CreditAttribution: acrollet commentedupdated patch attached, depends on the #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system.
Comment #27
acrollet CreditAttribution: acrollet commented#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.
Comment #28
Lars Toomre CreditAttribution: Lars Toomre commentedA 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,
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.
Comment #29
gdd"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.)
Comment #30
acrollet CreditAttribution: acrollet commentedpatch attached addressing item 1) in #28.
Comment #32
acrollet CreditAttribution: acrollet commentedarrgh, attaching patch without syntax error, thought I had fixed that.
Comment #33
jthorson CreditAttribution: jthorson commentedSorry, 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.
Comment #34
acrollet CreditAttribution: acrollet commentedThose 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.
Comment #36
jthorson CreditAttribution: jthorson commentedupdate_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.)
Comment #37
gddNow that update_variables_to_config() has been committed, this should work, so im going to requeue it.
Comment #38
gddComment #39
gdd#32: drupal-convert_rss_settings_to_config_system-1496462-32.patch queued for re-testing.
Comment #40
gddThis needs a reroll because system module update functions have moved on. Otherwise its probably ready to go.
Comment #41
cosmicdreams CreditAttribution: cosmicdreams commentedrerolled
Comment #43
cosmicdreams CreditAttribution: cosmicdreams commentedfixed the update function.
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedComment #46
gddCurrent patch doesn't apply because of system update method numbering, rolling a new one and looking into the remaining test fails.
Comment #47
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooking.
Comment #48
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRerolled 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.
Comment #49
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNext try.
Comment #50
gddI 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.
Comment #51
Dries CreditAttribution: Dries commentedGreat update, and glad to see more CMI-ification. Committed to 8.x.
Comment #53
sunFollow-up patch to adjust the new conversion for
#1599554: Tutorial/guidelines for how to convert variables into configuration
Comment #55
sunSorry. Re-rolled against HEAD.
Comment #57
sunWow, I'm really sorry. This one should pass.
Comment #59
ZenDoodles CreditAttribution: ZenDoodles commented#57: drupal8.config-rss.57.patch queued for re-testing.
Comment #60
ZenDoodles CreditAttribution: ZenDoodles commentedRetesting. Error seems unrelated to the patch:
Comment #62
sunAll config system conversions are API changes, so tagging as such.
Comment #63
aspilicious CreditAttribution: aspilicious commentedre-uploading
Comment #65
aspilicious CreditAttribution: aspilicious commentedOk 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.
Comment #66
sunThanks!
Comment #67
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!