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.
This is a sub-issue of #1775842: [meta] Convert all variables to state and/or config systems. This issue will convert the following queue variables to the config/state sub-systems:
- queue_class_ . $name
- queue_default_class
- queue_default_reliable_class
Comment | File | Size | Author |
---|---|---|---|
#20 | queue-dic-1814496-20.patch | 14.71 KB | Berdir |
#20 | queue-dic-1814496-20-interdiff.txt | 1.14 KB | Berdir |
#19 | queue-dic-1814496-19.patch | 14.61 KB | Berdir |
#19 | queue-dic-1814496-19-interdiff.txt | 7.69 KB | Berdir |
#16 | queue-dic-1814496-16.patch | 16.77 KB | Berdir |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an initial pass at a patch to convert these variables to the config system and the system.queue.yml file. Let's see what the bot thinks.
One open question I had is whether we need to test that the class actually implements QueueInterface. Hence, I added an @todo just before the assignment.
Comment #3
ruth_delattre CreditAttribution: ruth_delattre commentedComment #4
ruth_delattre CreditAttribution: ruth_delattre commentedComment #5
Lars Toomre CreditAttribution: Lars Toomre commentedAdding tags
Comment #6
ruth_delattre CreditAttribution: ruth_delattre commentedComment #7
ruth_delattre CreditAttribution: ruth_delattre commentedchanged:
1) variable names to: queue.class_.$name, queue.default_class and queue.default_reliable_class
2) variables_get to state()->get in common.inc and in the update function in system.install
3) updated variable names in function system_update_8031 and changed 31 to 33
Comment #8
ruth_delattre CreditAttribution: ruth_delattre commentedComment #10
ruth_delattre CreditAttribution: ruth_delattre commentedchanged:
1) variable names back to: class_.$name, class.default and class.default_reliable
2) state()->get to config(''system.queue)->get in common.inc and in the update function in system.install
3) function system_update_8033 and changed 33 to 34
4) changed {variable} to 'variable' in db_query statement in function system_update_8034
Comment #11
BerdirWe need to convert queue() to DIC with a factory class, just like other systems. I'm not sure how this will affect this, it's quite possible that it will actually just do a global $conf, see KeyValue factories.
I think thesw variables are usually used in a similar way as the cache backend configuration, meaning it's configured directly in settings.php. Unlike cache, it might be possible for them to be in actually stored variables, but I'm not sure if this is something that we need to support.
We should create a single mapping array and then call the helper function once.
Not sure if this is actually necessary.
Comment #13
ruth_delattre CreditAttribution: ruth_delattre commentedFrom what I understand (still a novice), converting queue-variables seems to be a little more complicated than writing an update routine. Thus, I am releasing the issue :-(
Comment #14
BerdirOk, here is an initial implementation.
Based on the abstracted factory pattern used by keyvalue and my cache in DIC issue.
I'm not exactly sure when we should use this and when the plugin system, possibly this should use the plugin system but the advantage of this pattern is that it allows a very clean definition in the container where everything is injected.
Comment #16
BerdirFixed the tests and the accidental change in the key value factory, this should pass the tests.
Wow, that was easier an expected :)
Comment #17
aspilicious CreditAttribution: aspilicious commentedExceeds 80 chars
\Drupal\Core\...
Add a newline between those two lines.
Add a newline between @param and @return
Small nitpicks, looking great
Comment #18
BerdirThanks for the review.
We were discussing about plugins in IRC, and when plugins should be used and when not. I wasn't sure myself where the distinction is what should be pluggable and for what the abstracted factory pattern from the keyvalue system is enough. So I came up with the following:
Unlike something like the views plugins, blocks, where configuration can be changed all the time, possibly by normal users, this feels more like system level configuration to me. To change this, keyvalue, cache backend (possibly mail as well?), additional configuration is usually required, something like Memcache or MongoDB or a queue application needs to be installed on a server first And it is environment specific, similar to the distinction between $settings and CMI (#1833516: Add a new top-level global for settings.php - move things out of $conf). There also won't be dozens of plugin definitions that need to be discovered and presented to the user.
Does that make sense? :)
Will re-roll this after the settings issue is in and also investigate if we can inject the Settings class, not sure yet if that will be possible.
Comment #19
BerdirOk.
- Fixed the mentioned coding style issues.
- Improved documentation in the settings class and added a getSingleton() method
- Added it to the container so that we can inject it into the QueueFactory.
=> No more globals :)
Comment #20
BerdirPer @chx's review in IRC, made Settings final (the alternative would to use static:: and not self:: but that's rather pointless, this class can not be extended so lets mark it as such) and changed $singleton to $instance.
Comment #21
chx CreditAttribution: chx commentedThis patch is really yummy and cleans up queue nicely.
Comment #22
andypostPlease fix doc-block comments according current classes
Overrides should be short className::method() following current http://drupal.org/coding-standards/docs#classes
Comment #23
BerdirThat was not changed or introduced in this patch. I just had to rename the class that Batch extends from. Coding standards should only be applied to code that is actually changed in a patch.
Also, the place you are referring to has not yet been agreed upon, the issue is still at needs work and being discussed.
Comment #24
catchLooks great! Committed/pushed to 8.x.
Comment #25
Berdirhttp://drupal.org/node/1479680 should be updated/extended...