Posted by alexpott on January 17, 2013 at 1:48am
27 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | configuration system |
| Category: | task |
| Priority: | major |
| Assigned: | catch |
| Status: | fixed |
| Issue tags: | Avoid commit conflicts, Configuration system |
Issue Summary
Problem/Motivation
- Leverage symfony's event model to make #1515312: Add snapshots of last loaded config for tracking whether config has changed, #1842956: Add hook_config_validate() and #1808248: Config import hooks of newly enabled extensions are not invoked in the config staging/import process easier
- Reduce the amount of procedural code in the Configuration managemet sub-system to make it more consistent
- config.inc contains a mish-mash of config_sync_*() and config_import_*0 functions
- Reduce amount of unnecessary config import / sync code loaded during normal runtime
Proposed resolution
- refactor code to create ConfigSync class to manage configuration import and installs
Remaining tasks
- needs validation of approach
- needs tests for Symfony events
Comments
#1
Here's an initial patch the converts config_import, config_install_default_config, and ViewTestData::importTestViews to use a new ConfigSync class to manage and perform the synchronisation tasks.
It also fixes a bug in ViewTestData::importTestViews - the current version only works for a single module in the modules array - fortunately we only ever pass it a single module.
#2
The last submitted patch, 1890784.drupal8.config-sync.patch, failed testing.
#4
This patch contains the fix from #1889752: Remove unnecessary manifest creation in config_install_default_config() and so was broken in the same way as explained in http://drupal.org/node/1889752#comment-6949574
Additionally the processed list was not correctly maintained so config entities were imported twice resulting in views with no uuid.
#5
The last submitted patch, 1890784.4.drupal8.config-sync.patch, failed testing.
#6
Doing a retest as install works locally for me.
#7
#4: 1890784.4.drupal8.config-sync.patch queued for re-testing.
#8
#4: 1890784.4.drupal8.config-sync.patch queued for re-testing.
#9
The last submitted patch, 1890784.4.drupal8.config-sync.patch, failed testing.
#10
Rerolled
#11
#12
The last submitted patch, 1890784-refactor-import-10.patch, failed testing.
#13
So four hours ago I was getting on a plane and the patch in #4 failed to apply. So I rerolled it, incorrectly, without the new files. So now I just started over with a fresh checkout and a fresh branch ... and the patch in #4 now magically applies. So I'm throwing it back to the bot.
#14
#4: 1890784.4.drupal8.config-sync.patch queued for re-testing.
#15
It conflicted with #1831264: Use the Entity manager to create new controller instances, rerolled.
#16
I'm not sure I see where this patch is a net win, but then again I don't see moving procedural code to objects as being a net win in and of itself in general. Is there a specific reason why this is better encapsulated into objects than just leaving it in the include file?
Some general comments:
- Calling a function called setChanges() with no parameters strikes me as odd. I'd actually expect the change list to be generated automatically when the ConfigSync object is constructed since in the majority of cases you'll just want to construct and sync. If you want to set a different change set, use setChanges() then.
- I think doSync() would make more sense just being named sync().
- I do like the switch to using events, as it makes our notification system more consistent with what we're doing for overrides.
#17
this isn't a switch to events. it's an addition of an event - nothing equivalent existed before this patch.
i agree with #16 - i don't see the benefit here with OO. i'd rather we dropped this patch entirely and changed it to just the change to add the sync event.
if we are going to OOify this, then i think we need to register this with the container, and inject the event dispatcher? and invoke Crell on it?
#18
I started this patch after spending some time thinking about the config validation patch. I realised that in order to mirror the rest of the Config sub system we'd need to use Symfony events to be more consistent. Once I was going down this route I wanted to pass the event something to act on and that could provide functions to get information about and manage the synchronisation - hence the conversion to OO.
I've been working on the addition of a validate event and moving the config name validation to an event subscriber to try to show the value of this approach. In doing this I've created a config_sync service on the container. This has turned out to have a significant benefit. At the moment when we press submit on the ConfigUI we calculate the changelist to build the form and then we rebuild it completely during config_import. Registering the config_sync service allows us to do this only once.
I think we might discover further advantages to an OO approach - for example - creating an event to be able to defer changes or reorder the changelist depending on dependencies.
#19
i'm mostly meh about the OO conversion, so i don't mind if it goes in. wouldn't be my first choice, but meh ;-)
if you want an event fired after import, lets discuss that.
if you want to implement deferring, reordering based on dependencies etc during import, lets discuss that.
but - can we please not push functional changes as side effects of implementation details?
#20
This patch:
All I'm trying to do is to refactor config import to point where we can solve some of the issues we have in the follow up patches. I was not saying that this patch should implement deferring or re-ordering. What I was trying to say is that we should try to make these easy to implement. In my opinion if we keep the current procedural approach this will lead to extra complexity.
The interdiff is bigger that the patch because some of the new classes have been renamed.
#21
I absolutely and totally agree that config.import/config.sync should be a service. @alexpott and I discussed this at DrupalCon Munich already, with the important conclusion that turning this into a classed service will be the only way for contrib/custom sites to invent a better configuration import/sync/staging implementation in D8. Procedural functions can't be swapped out.
And as things currently stand, contrib will have to re-invent in D8, in order to fix and close the gaps that we failed to address in core...
(I, for one, will probably work on an implementation that replaces all machine names with UUIDs in config names across the board, in order to truly guarantee uniqueness.)
So, thanks for kick-starting this, Alex. Will try to review this patch some more in-depth as soon as possible. Any chance you could push your code into the cmi sandbox? :)
#22
Branch pushed to CMI sandbox... http://drupalcode.org/sandbox/heyrocker/1145636.git/shortlog/refs/heads/...
#23
sun is correct. A straight conversion of procedural functions to injected DIC services automatically gets you:
1) Lazy-loading code.
2) Lazy-initializing services (when you need it it's initialized, but you don't spend time initializing until you need it).
3) Contrib can rip it out and replace it if it wants, as long as it follows the interface.
That's a strong case for service-ifying most systems all on its own, IMO.
I don't fully understand the problem space, but since I was invoked, some additional cleanliness/architectural comments below. From my incomplete read through, this does seem like a reasonably good use of events.
+++ b/core/lib/Drupal/Core/Config/ConfigImport.php@@ -0,0 +1,373 @@
+ public function __construct(StorageInterface $target_storage, EventDispatcher $event_dispatcher = NULL) {
+ $this->targetStorage = $target_storage;
+ $this->eventDispatcher = $event_dispatcher ? $event_dispatcher : drupal_container()->get('event_dispatcher');
+ $this->resetLists();
Don't bother with the hard coded drupal_container() call. If the event dispatcher is required, then it's required. Passing it in is what the DIC is for.
+++ b/core/lib/Drupal/Core/CoreBundle.php@@ -60,6 +60,12 @@ public function build(ContainerBuilder $container) {
+ // Register configuration synchronisation.
+ $container
+ ->register('config.import', 'Drupal\Core\Config\ConfigImport')
+ ->addArgument(new Reference('config.storage'))
+ ->addArgument(new Reference('event_dispatcher'));
Shouldn't this be config.importer? It's an object that does things, so it has an "er" suffix for thing it does.
+++ b/core/modules/config/config.admin.inc@@ -28,15 +28,16 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+ $config_import = drupal_container()->get('config.import')->setSourceStorage(drupal_container()->get('config.storage.staging'));
+ if (!$config_import->defaultChangelist()->hasChanges()) {
$form['no_changes'] = array(
This should be done in the container configuration. You can call:
<?php$definition->addMethodCall('setSourceStorage', new Reference('config.storage.staging'));
?>
And you're good.
If this is something that gets set per-use case, then it should either be multiple DIC registrations or a factory in the DIC, I think. (Although I don't know the specific use case in detail.)
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php@@ -180,8 +188,10 @@ function testNameValidation() {
+ drupal_container()->get('config.import')
+ ->setSourceStorage(drupal_container()->get('config.storage.staging'))
+ ->defaultChangelist()
+ ->import();
Ibid.
Or, really, this is in a test so you shouldn't be calling the container in the first place. Especially inside DUTB. Just make objects yourself and use 'em. That's how you keep unit tests unit-y.
#24
Thanks for the great review @crell!
Patch attached tries to implement all your suggestions:
Unfortunately the interdiff can't be constructed since this is a reroll.
Pushed rerolled branch to heyrocker's sandbox.
#25
The last submitted patch, 1890784.24.drupal8.config-sync.patch, failed testing.
#26
This failed on
Drupal\text\Tests\TextSummaryTestThe test did not complete due to a fatal error. Completion check TextSummaryTest.php 169 Drupal\text\Tests\TextSummaryTest->testOnlyTextSummary()
Does not occur locally - sending for a retest.
#27
#24: 1890784.24.drupal8.config-sync.patch queued for re-testing.
#28
Rerolled for #1515312: Add snapshots of last loaded config for tracking whether config has changed and #1889620: config_install_default_config() overwrites existing configuration.
Config snapshot creation is now an event subscriber to the import event.
#29
Forgot to attach diff of 24 to 28 - it's not a true interdiff as this was a reroll...
#30
+++ b/core/includes/config.incundefined@@ -24,28 +23,23 @@
+ drupal_container()->get('config.installer')->setSourceStorage($source_storage)
+ // Ignore manifest files when creating changelist to import.
+ ->defaultChangelist(FALSE)
+ // Only import new config. Changed config is from previous enables and
+ // should not be overwritten.
+ ->removeChangelist('delete')
+ ->removeChangelist('change')
+ ->import();
The
removeChangelistsucks...Patch attached fixes this and removes the config.installer service from the container as this is only used in one place and does not need to injected.
#31
Invoking https://twitter.com/chx/status/242326777538695168/photo/1 , trying to imagine what stof would do with this API.
What about
<?php$installer
->setSourceStorage($source_storage)
// Ignore manifest files when creating changelist to import. Only import
// new config. Changed config is from previous enables and should not be
// overwritten.
->useManifest(FALSE)
->addChangeList('create')
->import();
?>
#32
The last submitted patch, 1890784.30.drupal8.config-import.patch, failed testing.
#33
Thanks for the review @chx
#34
I think this is RTBC but as sun written this , he should review it.
#35
#33: 1890784.33.drupal8.config-import.patch queued for re-testing.
#36
The last submitted patch, 1890784.33.drupal8.config-import.patch, failed testing.
#37
Working on a reroll... Changes due to the config context patch :)
#38
Refactored the patch again due to the impact of context and other changes in core since this #33. This really requires a re-review... and interdiff is not possible because of the re-roll.
Summary of changes to #33:
#39
The last submitted patch, 1890784.38.drupal8.config-import.patch, failed testing.
#40
Okay some views test's have an unmet dependency on the theme system being configured...
And config factory reset needs to clear the static cache not just reinitialise the config. I think this is the correct thing to do because currently we are reinitialising all the config stored in the static cache on a reset() but we have no idea if we're actually going to use it again.
#41
#42
This one has gone a bit quiet. Do we still want it?
#43
I nIRC, @alexpott confirms that we still want this and he offerred to reroll tomorrow.
#44
Rerolled...
#45
After chatting to @timplunkett in IRC decided to remove the config_import function so everything is contained with the ConfigImporter class.
#46
The last submitted patch, 1890784.45.drupal8.config-import.patch, failed testing.
#47
Rerolled...
#48
This is beautiful. Once this is in and I reroll #1945398: Convert config_admin_import_form to a new FormInterface implementation and routing definition., it will be clear how nice this is. It really makes it clear how you can interact with the config import process, and will be immensely useful to contrib enhancements of this workflow.
I'll do a thorough review tonight.
#49
The last submitted patch, 1890784.47.drupal8.config-import.patch, failed testing.
#50
Messed up the reroll of core/lib/Drupal/Core/CoreBundle.php
Some more DrupalUnitTestBase tests have undeclared dependencies on the theme system...
Patched attached fixes both these issues.
#51
+++ b/core/includes/config.incundefined
@@ -38,22 +30,23 @@ function config_install_default_config($type, $name) {
+ $installer = new ConfigImporter(
+ 'config.installer',
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.phpundefined
@@ -45,21 +46,32 @@ function testSnapshot() {
+ $config_importer = new ConfigImporter(
+ 'snapshot.comparer',
+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestData.phpundefined
@@ -54,16 +48,27 @@ public static function importTestViews($class, $modules = array()) {
+ $installer = new ConfigImporter(
+ 'views.test.installer',
This makes me wonder if there is another way to pass the string to the service, so the constructor could be the same and you wouldn't need to always pass 5 of the same services
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -193,8 +202,26 @@ function testNameValidation() {
+ $config_importer = new ConfigImporter(
+ 'config.importer',
+ $this->container->get('config.storage'),
+ $this->container->get('event_dispatcher'),
+ $this->container->get('config.context.free'),
+ $this->container->get('config.factory'),
+ $this->container->get('plugin.manager.entity'),
+ $this->container->get('lock')
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.phpundefined
@@ -39,6 +47,17 @@ function setUp() {
+ $this->config_importer = new ConfigImporter(
+ 'config.importer',
+ $this->container->get('config.storage'),
+ $this->container->get('event_dispatcher'),
+ $this->container->get('config.context.free'),
+ $this->container->get('config.factory'),
+ $this->container->get('plugin.manager.entity'),
+ $this->container->get('lock')
These could be
$this->container->get('config.importer');, right?+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.phpundefined@@ -45,21 +46,32 @@ function testSnapshot() {
+ $snapshot,
+ drupal_container()->get('event_dispatcher'),
+ $this->container->get('config.context.free'),
$this->container
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestStorageController.phpundefined
@@ -26,13 +26,13 @@ public function importCreate($name, Config $new_config, Config $old_config) {
- * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importChange().
+ * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importUpdate().
*/
- public function importChange($name, Config $new_config, Config $old_config) {
+ public function importUpdate($name, Config $new_config, Config $old_config) {
// Set a global value we can check in test code.
$GLOBALS['hook_config_import'] = __METHOD__;
- return parent::importChange($name, $new_config, $old_config);
+ return parent::importUpdate($name, $new_config, $old_config);
This is a bit weird to change here, but I like the new name better anyway.
#52
+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined@@ -96,15 +96,13 @@ public function get($name) {
+ unset($this->cache[$cache_key]);
I have probably missed something but if it's a reset, can it not just set $this->cache = array()?
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@
+
+class ConfigImporter {
Docblock
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@
+ * List of config entities in the managed by manifests in the source storage.
huh? :)
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@
+ * Add changes to the changelist.
Adds
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@
+ $changes = array_diff($changes, $this->changelist[$op]);
+ $this->changelist[$op] = array_merge($this->changelist[$op], $changes);
Isn't it the same to += the two arrays together, as then only keys that aren't already set will be added?
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@
+ if (substr($name, 0, 9) != 'manifest.') {
This would be changed to
strpos($name, 'manifest.') === 0. It's all preference though I guess.+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,586 @@
+ if (count($this->getUnprocessed($op))) {
Seems strange doing count there? Would we usually do !empty()? Either way, this will work just fine.
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.phpundefined@@ -0,0 +1,48 @@
+ foreach (array('delete', 'create', 'update') as $op) {
+ foreach ($event->getConfigImporter()->getUnprocessed($op) as $name) {
This snippet gets used a few times, not sure if it's worth trying to abstract this out at all? Probably not actually.
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.phpundefined
@@ -0,0 +1,48 @@
+ $events['config.importer.validate'][] = array('onConfigImporterValidate', 40);
+ $events['config.installer.validate'][] = array('onConfigImporterValidate', 40);
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined
@@ -0,0 +1,68 @@
+ $events['config.importer.import'][] = array('onConfigImporterImport', 40);
Maybe we should add some class constants for this event string?
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined@@ -193,8 +202,26 @@ function testNameValidation() {
+ catch (ConfigNameException $e) {
+ $this->pass($message);
+ }
What happens if an exception is not thrown? The tests will still pass. What we have done in tests before is set a variable and then evaluate after the try/catch.
#53
Thanks for the reviews!
In reply to @timplunkett #51
1. @timplunkett "This makes me wonder if there is another way to pass the string to the service" ... suggestions welcome :)
2. Fixed - did it this way due to @crell's last comment in #23 ... but that was before so many dependencies
3. Fixed
4. I had change
importChangetoimportUpdateto match the $op's being stored in the changelists... create, delete and change just didn't seem right... have a look inConfigImporter::importInvokeOwner()In reply to @damienkloip #52
1. Yep you missed that here we're clearing the static cache of a specific config object not all... see
ConfigFactory::reset()2. Fixed
3. Fixed
4. Fixed
5. Nope we have numeric keys so this doesn't have the desired effect of unique array values.
6. The strpos version is not quite equivalent... the current version is also only testing the first 9 characters of the config name
7. You can't use
empty()to test function returns - see http://php.net/manual/en/function.empty.php8. I tried but I agree probably not :)
9. The point here is that the events are based on the ConfigImporter instance's servicename... I don't think these should be class constatns
10. Added a $this->fail() if the exception is not thrown.
Whilst adding the docblock for ConfigImporter (@damienkloip's point 2) I realised that the comparison of configuration stored in different config storage controllers is a different concern than actually doing the import. The attached patch implements a StorageComparerInterface and 2 classes that implement it ConfigComparer and CongirComparerManifest - the difference being that one uses manifests to decide what to do for config entities. An additional advantage of this approach can be seen in the ConfigSnapshotTest where comparing the snapshot storage to active and staging is very simple (contrast with the patch in #50).
#54
And one more thing...
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@
+ // Use an override free context for importing so that overrides to do not
+ // pollute the imported data. The context is hard coded to ensure this is
+ // the case.
+ $this->context = new FreeConfigContext($this->eventDispatcher);
After chatting with @beejeebus in IRC he pointed out that injecting a config context here made no sense as it only makes sense to run the import with a FreeConfigContext.
#55
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@
+ * @param \Drupal\Component\Plugin\PluginManagerInterface $entity_manager
+ * The entity plugin manager used to import config entities.
...
+ $this->pluginManager = $plugin_manager;
I think entityManager is a better name here, and I think its wrong to typehint with an interface that doesn't have the method we want on it. Either we need an interface for EntityManager::getStorageController(), or we should just typehint with the class.
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@
+ if ($entity_type = config_get_entity_type_by_name($name)) {
+ $old_config = new Config($name, $this->storageComparer->getTargetStorage(), $this->context);
+ $old_config->load();
+
+ $data = $this->storageComparer->getSourceStorage()->read($name);
+ $new_config = new Config($name, $this->storageComparer->getTargetStorage(), $this->context);
+ if ($data !== FALSE) {
+ $new_config->setData($data);
+ }
+
+ $method = 'import' . ucfirst($op);
+ $handled_by_module = $this->pluginManager->getStorageController($entity_type)->$method($name, $new_config, $old_config);
+ }
+ if (!empty($handled_by_module)) {
+ $this->setProcessed($op, $name);
This is way out of scope for this issue, but the coupling of CMI to Config Entities here always bothered me greatly. Is it possible that the ConfigEntity system could subscribe to the event instead (in a follow-up).
It would break the $handled_by_module part, but I'm still unsure if we need that.
+++ b/core/lib/Drupal/Core/Config/StorageComparer.phpundefined
@@ -0,0 +1,203 @@
+ * {@inheritdoc}
+ */
+ public function __construct($source_storage, $target_storage) {
+++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.phpundefined
@@ -0,0 +1,130 @@
+ /**
+ * Constructs the Configuration storage comparer.
+ *
+ * @param \Drupal\Core\Config\StorageInterface $source_storage
+ * Storage controller object used to read configuration.
+ * @param \Drupal\Core\Config\StorageInterface $target_storage
+ * Storage controller object used to read configuration.
+ */
+ public function __construct($source_storage, $target_storage);
Needs typehinting, and we should move it out of the interface (we don't usually put __construct in interfaces)
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined@@ -0,0 +1,68 @@
+ static function getSubscribedEvents() {
public?
#56
@timplunkett / #55
1. Agreed and fixed
2. Yep we could add an event which always modules to do handle an import instead of the general process - definitely out of scope here.
3. Fixed
4.
static function getSubscribedEvents() {is how all the other core events are...#57
This looks awesome. Thanks so much @alexpott!
#58
alexpott asked me to give this a standards look.
+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined
@@ -0,0 +1,348 @@
+ * @see \Drupal\Core\Config\StorageComparerInterface
...
+ * @see \Drupal\Core\Config\ConfigImporterEvent
usually @see's are together at the end of the doc block, but iirc, they can also be placed inline if that helps communicate better.
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@
+ * Each instance of ConfigImporter has a service name which is used to construct
+ * event names. The events fired during an import are:
+ * - '{service name}.validate': Events listening can throw a
+ * \Drupal\Core\Config\ConfigImporterException to prevent an import from
small change in formatting for lists in comments.
http://drupal.org/node/1354#lists
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@
+ * The name of the ConfigImporter. Used to identify events.
...
+ * The storage comparer used to discover configuration changes.
...
+ * The event dispatcher used to notify subscribers.
changed to be consistant.
. Used ...
->
used
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@
+ * @var \Drupal\Core\Lock\LockBackendInterface $lock
taking out the $lock
http://drupal.org/node/1354#var
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,348 @@
+ * Changelists to check for changes. Defaults to all changelists, i.e.
+ * array('delete', 'create', 'update').
...
+ public function hasChanges($changelists = array('delete', 'create', 'update')) {
...
+ * @param string $op
+ * The change operation performed. Either create, change or delete.
...
+ * @param $op
+ * The change operation to get the unprocessed list for. Either create,
+ * update or delete.
one missing type in @param.
also, one has change instead of update.
Making them match (and also match the order in the hasChanges definition:
hasChanges($changelists = array('delete', 'create', 'update'))
=======
posting what I have so far. I can get back to this in a couple hours.
#59
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined
@@ -0,0 +1,347 @@
+ /**
+ * Checks if there are any unprocessed changes.
+ *
+ * @param array $changelists
+ * Changelists to check for changes. Defaults to all changelists, i.e.
+ * array('delete', 'create', 'update').
+ *
+ * @return bool
+ * TRUE if there are changes to process and FALSE if not.
+ */
+ public function hasChanges($changelists = array('delete', 'create', 'update')) {
+ foreach ($changelists as $op) {
+ if (count($this->getUnprocessed($op))) {
+ return TRUE;
+ }
+ }
+ return FALSE;
+ }
+++ b/core/lib/Drupal/Core/Config/StorageComparer.phpundefined
@@ -0,0 +1,208 @@
+ /**
+ * Checks if there is a changelist with changes to process.
+ *
+ * @param array $ops
+ * Operation to check for changes. Defaults to all operations, i.e.
+ * array('delete', 'create', 'update').
+ *
+ * @return bool
+ * TRUE if there are changes to process and FALSE if not.
+ */
+ public function hasChanges($ops = array('delete', 'create', 'update')) {
+ foreach ($ops as $op) {
+ if (!empty($this->changelist[$op])) {
+ return TRUE;
+ }
+ }
+ return FALSE;
+ }
+++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.phpundefined
@@ -0,0 +1,120 @@
+ /**
+ * Checks the changelist has any changes.
+ *
+ * Until the changelist has been calculated this will always be FALSE.
+ *
+ * @see \Drupal\Core\Config\StorageComparerInterface::createChangelist().
+ *
+ * @param array $changelists
+ * Changelists to check for changes. Defaults to all changelists, i.e.
+ * array('delete', 'create', 'update').
+ *
+ * @return bool
+ * TRUE if there are changes to process and FALSE if not.
+ */
+ public function hasChanges($ops = array('delete', 'create', 'update'));
+
There is the config importer, comparer, and interface.
I updated to be more consistant, I wonder if it can use a {@inheritdoc}.
+++ b/core/lib/Drupal/Core/Config/StorageComparer.phpundefined@@ -0,0 +1,208 @@
+ * @param \Drupal\Core\Config\StorageInterface $source_storage
+ * Storage controller object used to read configuration.
+ * @param \Drupal\Core\Config\StorageInterface $target_storage
+ * Storage controller object used to read configuration.
updating target comment to be "to write".
+++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.phpundefined@@ -0,0 +1,120 @@
+ * Gets the configuration target storage.
+ *
+ * @return \Drupal\Core\Config\StorageInterface
+ * Storage controller object used to read configuration.
+ */
+ public function getTargetStorage();
fixed copy and paste error. "to read" -> "to write".
+++ b/core/modules/config/config.admin.incundefined@@ -21,7 +22,7 @@
* @param Drupal\Core\Config\StorageInterface $target_storage
* The target storage to compare differences to.
*/
-function config_admin_sync_form(array &$form, array &$form_state, StorageInterface $source_storage, StorageInterface $target_storage) {
+function config_admin_sync_form(array &$form, array &$form_state, StorageInterface $source_storage) {
needed to delete the @param for target that was removed.
this is also missing the @return.
Maybe this is an unrelated change?
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestStorageController.phpundefined@@ -26,13 +26,13 @@ public function importCreate($name, Config $new_config, Config $old_config) {
- * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importChange().
+ * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importUpdate().
I think this can be an {@inheritdoc} but maybe this was left here to be part of cleaning up this later. So I'll leave it.
+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestData.phpundefined@@ -7,6 +7,8 @@
+use Drupal\Core\Config\ConfigImporter;
+use Drupal\Core\Config\StorageComparer;
use Drupal\Core\Config\FileStorage;
I think these are usually alphabetical.
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,347 @@
+ /**
+ * Invokes import* methods on configuration entity storage controllers.
+ *
+ * The changelist is modified as each change is processed.
+ *
+ * @todo Add support for other extension types; e.g., themes etc.
+ */
+ protected function importInvokeOwner() {
+ // Allow modules to take over configuration change operations for
+ // higher-level configuration data.
+ // First pass deleted, then new, and lastly changed configuration, in order to
+ // handle dependencies correctly.
The Allowed... sentence seems about the function in general so moved it up to the function doc block.
+++ b/core/lib/Drupal/Core/Config/ConfigImporterEvent.phpundefined@@ -0,0 +1,39 @@
+ return $this->configImporter;
+ }
+}
added a new line at the end of the class because, mostly we do, and also, other classes in this patch do.
#60
I made the small change about the doc block and changed it to an {@inheritdoc} like YesCT sugjested in comment #59.
#61
Talking to alexpott and EllaTheHarpy in irc, we agree the hasChanges in ConfigImporter is different than the hasChanges in ConfigComparer, so renaming it with a better name: hasUnprocessedChanges.
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined
@@ -0,0 +1,68 @@
diff --git a/core/modules/config/config.admin.inc b/core/modules/config/config.admin.inc
+++ b/core/modules/config/config.admin.incundefined
@@ -31,18 +33,20 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+ $config_import = Drupal::service('config.importer');
+ if (!$config_import->hasChanges()) {
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined
@@ -87,7 +97,7 @@ function testDeleted() {
+ $this->assertFalse($this->configImporter->hasChanges());
@@ -138,7 +148,7 @@ function testNew() {
+ $this->assertFalse($this->configImporter->hasChanges());
@@ -195,7 +205,7 @@ function testUpdated() {
+ $this->assertFalse($this->configImporter->hasChanges());
I dont see a hasChanges in any of the use classes. Why does this work?
Is it because it's on an object of type ConfigImporter?
Then it might should be the new name hasUnprocessedChanges()
#62
The last submitted patch, 1890784.61.drupal8.config-import.patch, failed testing.
#63
+++ b/core/modules/config/config.admin.incundefined
@@ -31,18 +33,20 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+ if (!$config_import->hasChanges()) {
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined
@@ -87,7 +97,7 @@ function testDeleted() {
+ $this->assertFalse($this->configImporter->hasChanges());
@@ -138,7 +148,7 @@ function testNew() {
+ $this->assertFalse($this->configImporter->hasChanges());
@@ -195,7 +205,7 @@ function testUpdated() {
+ $this->assertFalse($this->configImporter->hasChanges());
I think these also needed switching?
#64
yeah, I convinced myself, and the testbot confirms. I'll do it. (since I broke it *grin*)
#65
Here it is.
#66
Reviewing the improvements made by YesCT and EllaTheHarpy I discovered a mistake I made in the documentation... well at one point it was true but now it a LIE :)
#67
Great docs and naming improvements!
And the code still looks great.
#68
looks good to me too.
#69
Very minor
+++ b/core/includes/config.incundefined@@ -425,6 +195,21 @@ function config_typed() {
+ * @param Drupal\Core\Config\StorageInterface $target_storage
+ * The storage to synchronize configuration to.
+ */
+function config_import_create_snapshot(StorageInterface $source_storage, StorageInterface $snapshot_storage) {
+ $snapshot_storage->deleteAll();
param names don't match :(
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigSnapshotSubscriber.phpundefined@@ -0,0 +1,68 @@
+ * Create config snapshot.
Creates?
#70
i think is ready to go.
rerolled for the doc only changes in #69.
#71
This is likely to have a conflict with #1987660: Convert config_sync() to a new style controller. This should be committed first as the other issue is just moving code hunks it isn't rtbc but just in case.
#72
#73
Talking with @catch about this... he didn't like passing the service name in... and I think @timplunkett wasn't sure about it either.So I've implemented this using class constants.
#74
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined
@@ -220,9 +217,9 @@
+ if (!$this->lock->acquire($this::SERVICE_NAME)) {
...
+ throw new ConfigImporterException(sprintf('Import service %s is already running', $this::SERVICE_NAME));
@@ -230,7 +227,7 @@
+ $this->lock->release($this::SERVICE_NAME);
@@ -318,7 +315,7 @@
+ $this->eventDispatcher->dispatch($this::SERVICE_NAME . '.' . $event_name, new ConfigImporterEvent($this));
@@ -328,7 +325,7 @@
+ return !$this->lock->lockMayBeAvailable($this::SERVICE_NAME);
@@ -338,7 +335,7 @@
+ return $this::SERVICE_NAME;
Shouldn't this be
static::SERVICE_NAME? That's what we use elsewhere.#75
Yep... doh!
#76
RTBC if green! This feels much better.
#77
it hurts my head to ask this question, but:
<?php
public function import() {
if ($this->hasUnprocessedChanges()) {
// Ensure that the changes have been validated.
$this->validate();
$this->configFactory->enterContext($this->context);
?>
does this mean that validation and import run against different config contexts? it hurts my head because i don't really know what that means, but my gut feeling is that is probably not a good idea.
#78
@beejeebus I did consider this but the reason I did not do this is that I think it should the event listener's job to worry about context if it needs to... If it does it is probably incorrect :)... Because validators should be using the storagecontrollers attached to the storagecomparer to read config in. After all most validators are probably not going to look at what's there but what's coming and therefore will have to read from staging. Additionally what they are most likely to want to compare to active is stuff like machine names and uuid's and context messes will these... well... all bets are off.
#79
Looks much better with the constants, a couple of things still confuse me bit:
Installing default config is sufficiently rare that I completely agree it doesn't deserve it's own service, however it's weird that there's a StorageComparer service and the class is also instantiated directly - does it need to be a service at all then, why not instantiate directly everywhere?
+++ b/core/includes/config.incundefined@@ -38,22 +31,19 @@ function config_install_default_config($type, $name) {
+ $storage_comparer = new StorageComparer($source_storage, Drupal::service('config.storage'));
+ // Only import new config. Changed config is from previous enables and
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined@@ -0,0 +1,341 @@
+ * Each instance of ConfigImporter has a service name which is used to construct
Class constant rather than a property on the instance.
+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.phpundefined@@ -0,0 +1,25 @@
+class ConfigInstaller extends ConfigImporter {
+
+ /**
+ * The name used to identify events and the lock.
+ */
+ const SERVICE_NAME = 'config.installer';
+
This defines a service name, but it's not registered as a service. I don't think these are service names at all, but rather operation names or similar.
#80
Removed the services added by the patch... Catch is right these will not be used often enough to justify their existence.
Renamed SERVICE_NAME to ID as that's what it is an identifier.
#81
Having them as a service allows the importing to be swapped out. I'm not certain enough of our choices to say that contrib won't have some better ideas.
However, I leave this to catch. I'm fine with either, just worried a bit.
#82
I think if we allow the importing to be swapped out, we'd also want to allow the defaults installation to be swapped out as well. It's the mismatch rather than them being services that bothers me (although I can't see another reason than swapping the implementation for them to be such).
This feels like something we could change at any time but leaving this open for a bit longer in case people want to discuss.
#83
Went ahead and committed/pushed this - if we want to make these services that'll be a simple follow-up - they're only called a couple of places.