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.

Files: 
CommentFileSizeAuthor
#4 1887244-4-use-config-methods.patch922 bytesbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 49,666 pass(es).
[ View ]
#1 1887244-use-config-methods.patch807 bytesbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 49,606 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new807 bytes
FAILED: [[SimpleTest]]: [MySQL] 49,606 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

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

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.

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

StatusFileSize
new922 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,666 pass(es).
[ View ]

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.

Assigned:Unassigned» heyrocker

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

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.

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?

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.

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.