currently, we use the methods on the storage controller, not the object, for non-config-entity config objects.

this stops us from reacting to changes to config objects from this code path, which is blocking other issues.

CommentFileSizeAuthor
#4 1887244-4-use-config-methods.patch922 bytesAnonymous (not verified)
#1 1887244-use-config-methods.patch807 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Status: Active » Needs review
FileSize
807 bytes

here's a patch, let's see what breaks.

chx’s picture

This would be fantastic and is a duplicate of at least two issues , one is the where we wanted to reintroduce hooks on import -- no need, just use the events the other is my meta whining. And, it would unblock ConfigEntities querying because we could use some wrapping of storage engines similar to the cached storage to write a flattened version (key1.key2.key3 - value) to the database allowing for quick querying with EntityQuery. Superb exciting.

sun’s picture

+++ b/core/includes/config.inc
@@ -184,12 +184,14 @@ function config_sync_changes(array $config_changes, StorageInterface $source_sto
       Config::validateName($name);
+      $config = new Config($name, $target_storage);

1) With this we can skip the manual validateName(), since that's baked into Config.

2) Why don't we use the service instead of manually futzing with Config?

+++ b/core/includes/config.inc
@@ -184,12 +184,14 @@ function config_sync_changes(array $config_changes, StorageInterface $source_sto
+        $config->setData($data ? $data : array());

Not really happy about another/new usage of setData() here... setData() is going to be incompatible with any kind of context/typed-property system.

But OK, it's not the only setData() in core and this one doesn't make the situation worse.

Anonymous’s picture

1) With this we can skip the manual validateName(), since that's baked into Config.

ah, good point, fixed.

2) Why don't we use the service instead of manually futzing with Config?

because that's what we do elsewhere in this code, see config_import_invoke_owner(). please open another issue if you want to change that, i'm not introducing anything new with this.

chx’s picture

Assigned: Unassigned » gdd

I would RTBC this but the decision is heyrocker's.

gdd’s picture

Status: Needs review » Reviewed & tested by the community

I feel like there is a reason we weren't doing this in the first place but I can't remember what it was and I'm not finding any references in past issues and tests pass so lets go with it. I will close #1886478: Bring back hook_config_import_CRUD() hooks as a duplicate, since non-config-entities can now act on the events instead.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

"this stops us from reacting to changes to config objects from this code path..."

Shouldn't we have a test for that then?

gdd’s picture

Status: Needs work » Reviewed & tested by the community

The unit tests for import are complete and working, whether or not the event is properly being fired should be covered by unit tests around the event system. Unfortunately we don't have those yet, so I filed #1889474: Create tests for config event handling as a major followup. Going back to RTBC here if webchick is OK with that solution.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Ok cool, I think that works.

Committed and pushed to 8.x. Thanks!

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