It's a follow-up from discussion #1588422-120: Convert contact categories to configuration system

problem/motivation

Currently each of Config entities needs to implement

+++ b/core/modules/contact/contact.moduleundefined
@@ -147,21 +147,87 @@ function _contact_personal_tab_access($account) {
 /**
+ * Implements MODULE_config_import_create().
+ */
+function contact_config_import_create($name, $new_config, $old_config) {
+  if (strpos($name, 'contact.category.') !== 0) {
+    return FALSE;
+  }
+
+  $category = entity_create('contact_category', $new_config->get());
+  $category->save();
+  return TRUE;
+}
+
+/**
+ * Implements MODULE_config_import_change().
+ */
+function contact_config_import_change($name, $new_config, $old_config) {
+  if (strpos($name, 'contact.category.') !== 0) {
+    return FALSE;
+  }
+
+  list(, , $id) = explode('.', $name);
+  $category = entity_load('contact_category', $id);
+
+  $category->original = clone $category;
+  foreach ($old_config->get() as $property => $value) {
+    $category->original->$property = $value;
+  }
+
+  foreach ($new_config->get() as $property => $value) {
+    $category->$property = $value;
+  }
+
+  $category->save();
+  return TRUE;
+}
+
+/**
+ * Implements MODULE_config_import_delete().
+ */
+function contact_config_import_delete($name, $new_config, $old_config) {
+  if (strpos($name, 'contact.category.') !== 0) {
+    return FALSE;
+  }
+
+  list(, , $id) = explode('.', $name);
+  entity_delete_multiple('contact_category', array($id));
+  return TRUE;

Proposed solution

- Rewrite config_import_invoke_owner()
- Implement helpers to minify code duplication

function foo_config_import_create($op, $name, $new_config, $old_config) {
  // work out your entity type from $name
  entity_thingie_config_create_helper($entity_type, $new_config, $old_config);
}

function entity_thingie_config_create_helper($entity_type, $new_config, $old_config) {
  entity_create($entity_type, $new_config->get())->save();
  return TRUE;
}
Files: 
CommentFileSizeAuthor
#24 interdiff.txt3.89 KBtim.plunkett
#23 config-import-hooks-1806178-23.patch22.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,000 pass(es). View
#17 interdiff.txt7.32 KBtim.plunkett
#17 die-import-hooks-1806178-14.patch23.02 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,974 pass(es). View
#14 die-import-hooks-1806178-14.patch21.12 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,774 pass(es), 2 fail(s), and 1 exception(s). View

Comments

sun’s picture

Title: Minimize code duplication for config entities » Remove code duplication for hook_config_import_*() implementations of config entities
Issue tags: +Configurables

The battle plan was to implement these hooks for a couple of more Configurables, and only afterwards check which parts of the implementations are actually identical, which parts are similar/equal-ish, and which parts need to remain in individual implementations.

heyrocker’s picture

One of the things that doesn't happen now is that if you don't implement these hooks, then no entity save/delete/update can happen, which means no entity hooks get fired. We also need some stuff to happen on all ConfigEntity save/delete for managing the manifest files in #1697256: Create a UI for importing new configuration. Ultimately the technical problem is that in config_import(), we don't know whether we are importing config managed by a ConfigEntity or not.

An idea I had is that we should formalize a naming convention that everyone is already pretty much doing

$module . $entity_info_key . $prefix . $id . yml

If we make this a mandatory standard, then in the import function we can do get_entity_info($entity_info_key). If we get something back, then instantiate the appropriate ConifgEntity and call the function at hand. Then a lot of this boilerplate code can go in ConfigEntityBase. We could also, if we wanted, eliminate the config_prefix (or at least make it optional or autogenerated?)

Discuss

sun’s picture

We don't need to "guess", because the config prefixes are precisely defined in entity plugin info/metadata.

heyrocker’s picture

But how do we get that entity info if we don't actually know what the ID is? For instance, right now, how would I know that image.style.large.yml is an instance of the 'image_style' entity?

beejeebus’s picture

yeah, i'd love to kill config_import_invoke_owner(), and go back to just invoking generic import hooks. like we did in the import patch pre-Drupalcon Denver, back in the good old days of simple CMI.

right now, modules can't a) reject a config import they know is broken before the damage is done or b) react to config import unless they are an 'owner'. sad, sad panda.

i don't care about keeping or killing 'config_prefix' or enforcing a naming convention, so whatever consensus we reach works for me.

just played around with using config_prefix, some completely untested code below (just swap out config_import_invoke_owner()).

looking at some of the implementations, it seems the ConfigEntity API is simply not enough to avoid config import hooks. image_style_delete(), for example, still needs to get called - $style->delete() is not enough. i don't really understand ConfigEntity, and was opposed to the idea, so i'm not in a good position to know if this is an anti-pattern, or if i was just off track thinking that ConfigEntity API would actually help us here.


function config_import_process_config_entities($config_changes, $source_storage, $target_storage) {
  $config_entity_types = array_filter(entity_get_info(), function ($entity_info) {
    return is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface');
  });

  foreach ($config_entity_types as $entity_type => $entity_info) {
    foreach (array('delete', 'create', 'change') as $op) {
      $config_entity_names = array_filter($config_changes[$op], function($config_name) use ($entity_info) {
        return strpos($entity_info['config_prefix'], $config_name) === 0;
      });
      foreach ($config_entity_names as $config_name) {
        switch ($op) {
          case 'create':
            $config_entity = entity_create($entity_type, $source_storage->read($config_name));
            $config_entity->save();
            break;
          case 'change':
            list(, , $id) = explode('.', $config_name);
            $config_entity = entity_load($entity_type, $id);
            $config_entity->original = clone $config_entity;
            foreach ($target_storarge->read($config_name) as $property => $value) {
              $config_entity->original->$property = $value;
            }
            foreach ($source_storage->read($config_name) as $property => $value) {
              $config_entityconfig_entity->$property = $value;
            }
            $config_entity->save();
            break;
          case 'delete':
            list(, , $id) = explode('.', $config_name);
            $config_entity = entity_load($entity_type, $id);
            $config_entity->delete();
            break;
        }   
      } 
      unset($config_changes[$op][$config_name]);
    }
  }
  return $config_changes;
}
beejeebus’s picture

Issue tags: +Configuration system

tagging.

heyrocker’s picture

looking at some of the implementations, it seems the ConfigEntity API is simply not enough to avoid config import hooks. image_style_delete(), for example, still needs to get called - $style->delete() is not enough.

Calling $style->delete() will trigger hook_ENTITY_TYPE_delete() so image_style_delete() gets called through that mechanism.

beejeebus’s picture

re #7 - huh, ok then i don't know what image_config_import_delete() is doing then...

sun’s picture

@beejeebus: Image module is a particularly bad example to look at. The entire code is about to be corrected in #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity)

moshe weitzman’s picture

From beejeebus in #5:

right now, modules can't a) reject a config import they know is broken before the damage is done or b) react to config import unless they are an 'owner'. sad, sad panda.

i'm sad about both of those as well. Would you guys support a patch to add those operations. I know we have discussed them a bit elsewhere but the conclusions in those monster issues was not clear.

sun’s picture

@moshe: Happy to discuss in a separate issue (both hooks aren't really related to this issue). The reason for why I removed the validation hook from the original config import patch was that I was not able to document it; i.e., I wasn't able to see how an import could reasonably work if a module rejects the import of one or more changes, and, how a module would determine that, and, why a module would want to do that in the first place. So we'd need to make sure to clarify the actual use-case(s) for those hooks in the new issue.

That said, the second hook b) to react on imported config unless owner might not be needed — thus far, everything that goes through hook_config_import_*() hooks are config entities, and config entities go through Entity API, which in turn invokes insert/update/delete hooks. But perhaps I'm also misunderstanding, and you're asking for a hook that is invoked for all config objects during/after import — in that case, that wouldn't be related to config entities.

moshe weitzman’s picture

OK, lets talk about validation at #1842956: [Meta] Implement event listeners to validate imports. I'm not ready to start a discussion around post_save right now.

tim.plunkett’s picture

Component: configuration system » configuration entity system
tim.plunkett’s picture

Status: Active » Needs review
FileSize
21.12 KB
FAILED: [[SimpleTest]]: [MySQL] 49,774 pass(es), 2 fail(s), and 1 exception(s). View

How about something along these lines?
Still needs some love, but it's worth a testbot run.

tim.plunkett’s picture

+++ b/core/includes/config.incundefined
@@ -253,12 +253,10 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
   foreach (array('delete', 'create', 'change') as $op) {

@@ -268,7 +266,8 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+        $method = "import_$op";

Obviously import_* doesn't fly as a method name, since it isn't lowerCamelCase, but I didn't get to changing the case on this everywhere yet

+++ b/core/includes/config.incundefined
@@ -297,3 +296,19 @@ function config_get_module_config_entities($module) {
+function config_get_config_entity_by_name($name) {

I chose to make this a standalone function, because it seems helpful. But, it could be moved into the above code, and entity_get_info() could be called less...

Status: Needs review » Needs work

The last submitted patch, die-import-hooks-1806178-14.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
23.02 KB
PASSED: [[SimpleTest]]: [MySQL] 49,974 pass(es). View
7.32 KB

It would help if I actually used the new controllers.

Also, just using ucfirst to fix the method names, and restoring the docblock above image_style_delete() (for #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity))

beejeebus’s picture

nice work! this patch looks good, mostly just a straight port of the code and tests from functions to OO. if it comes back green, i think it's RTBC.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

yay

andypost’s picture

+1 to RTBC, We need this to continue on #1814916: Convert menus into entities - Menu entity is defined by system module so menu.*.yml files needs to live not in menu module

tim.plunkett’s picture

Priority: Normal » Major

This blocks conversions to ConfigEntity, so bumping.

sun’s picture

Status: Reviewed & tested by the community » Needs review

By adding these methods to the ConfigStorageController and unconditionally invoking them, they seem to be part of a expected contract, but we're missing a corresponding ConfigEntityStorageControllerInterface, no?

I guess we can do this for now, but we should create a follow-up issue to discuss how we can introduce the additional methods in an interface - potentially by adding a separate Config\Entity\ImportableInterface, and checking for that before trying to invoke the methods.

Aside from that:

+++ b/core/includes/config.inc
+      if ($entity_type = config_get_config_entity_by_name($name)) {

+ * Returns the entity type of a configuration object.

Can we rename this to config_get_entity_type_by_name()?

+++ b/core/includes/config.inc
@@ -297,3 +296,19 @@ function config_get_module_config_entities($module) {
+    return (isset($entity_info['config_prefix']) && strpos($name, $entity_info['config_prefix']) === 0);

The config_prefix needs to be suffixed with a trailing dot/period to prevent false-positive matches; e.g., strpos $name "node" in config prefix "node_attach" would return TRUE, even though it must return FALSE.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -425,4 +426,91 @@ protected function invokeHook($hook, EntityInterface $entity) {
+   * Modules should implement this callback if they manage configuration data
+   * (such as image styles, node types, or fields) which needs to be
+   * prepared and passed through module API functions to properly handle a
+   * configuration change.

This part of the phpDoc comments on the new/moved methods can be dropped, I think.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -425,4 +426,91 @@ protected function invokeHook($hook, EntityInterface $entity) {
+    list(, , $id) = explode('.', $name);

As explained in #1831774-7: Config import assumes that 'config_prefix' contains one dot only and #9, we need to take the actual amount of dots/parts in the config prefix into account when determining the ID of a config entity in a config object name. It is not guaranteed and nowhere enforced that config prefix should contain exactly one dot only.

We could move that operation into a separate getEntityIDFromName() method, which could additionally perform the calculation of which part to extract only once per entity type controller instance (i.e., save it in a private property).

+++ b/core/modules/config/config.api.php
@@ -11,116 +11,3 @@
-  // @todo image_style_delete() supports the notion of a "replacement style"
-  //   to be used by other modules instead of the deleted style. Essential!
-  //   But that is impossible currently, since the config system only knows
-  //   about deleted and added changes. Introduce an 'old_ID' key within
-  //   config objects as a standard?

Can we keep + move this @todo, please? It's still one of the few remaining major unresolved tasks in the config entity system.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleStorageController.php
@@ -0,0 +1,34 @@
+/**
+ * @todo.
+ */
+class ImageStyleStorageController extends ConfigStorageController {

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php
@@ -18,7 +18,7 @@
- *   controller_class = "Drupal\Core\Config\Entity\ConfigStorageController",
+ *   controller_class = "Drupal\image\ImageStyleStorageController",

This implementation does not contain an actual difference to ConfigStorageController, except for the @todo. I'd suggest to keep the @todo in ConfigStorageController only and remove the special ImageStyleStorageController. The @todo should be resolved for the generic ConfigStorageController anyway.

+++ b/core/modules/views/views.module
-function views_config_import_create($name, $new_config, $old_config) {

-function views_config_import_delete($name, $new_config, $old_config) {

Humm... there's no hook_config_import_change() for views...? Odd. ;)

tim.plunkett’s picture

FileSize
22.3 KB
PASSED: [[SimpleTest]]: [MySQL] 50,000 pass(es). View

Yes, we'll need a new interface.

Renamed the function.

Added the dot to the prefix.

Removed the php doc.

I'm not touching the list/explode pairing, that's just moved code, and best solved in the other issue.

I've moved that docblock to ImageStyleStorageController, which is the reason it exists. It calls image_style_delete($entity), not $entity->delete(). See #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity)

tim.plunkett’s picture

FileSize
3.89 KB

Forgot the interdiff

sun’s picture

Thanks!

I'm not touching the list/explode pairing, that's just moved code, and best solved in the other issue.

It was valid in the individual hook implementations, because those implementations knew exactly how their config entity object names are structured.

The moment the list/explode is moved into an abstract/generic part of the system, it no longer has that knowledge and thus it cannot make that assumption anymore.

Therefore, we need to touch it for this issue/patch. The other/existing issue affects config manifest files only, so the impact of that bug is less severe than the impact of this patch would be; i.e., parsing the utterly wrong entity ID out of a config object name (and saving it), even though we managed to get to the entity type's storage controller already.

tim.plunkett’s picture

Every single config entity has only one dot currently. So this works fine, and doesn't change anything.

#1831774: Config import assumes that 'config_prefix' contains one dot only is still valid for protecting against future issues, but it's still not a problem now.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

yeah, i agree with #26, lets deal with the config prefix stuff in another issue. changes look good, back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/includes/config.incundefined
@@ -268,7 +266,8 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+        $method = 'import' . ucfirst($op);

Hm. Is that robust enough? I suppose so, since the ops are a known list.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -425,4 +426,76 @@ protected function invokeHook($hook, EntityInterface $entity) {
+  public function importCreate($name, Config $new_config, Config $old_config) {
...
+    $entity->save();
+    return TRUE;
...
+  public function importChange($name, Config $new_config, Config $old_config) {
...
+    $entity->save();
+    return TRUE;
...
+  public function importDelete($name, Config $new_config, Config $old_config) {
...
+    $entity->delete();
+    return TRUE;

It seems like these shouldn't be unconditionally returning TRUE, but rather returning the success of the operation.

Also missing @return values in the PHPDoc.

...oh, nuts. I see that this is coming directly over from deleted code in hook_config_import_FOO(). :( Can we get a follow-up issue to clean this up?

Since those were the only two things, committed and pushed to 8.x. Thanks!

heyrocker’s picture

Spawned #1886478: Bring back hook_config_import_CRUD() hooks as a followup to this issue.

alexpott’s picture

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