Remove code duplication for hook_config_import_*() implementations of config entities

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.

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

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

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?

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: 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)

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.

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

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.

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

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

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))

This blocks conversions to ConfigEntity, so bumping.

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. ;)

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)

Forgot the interdiff

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.

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.

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

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

Spawned #1889752: Remove unnecessary manifest creation in config_install_default_config() as a follow up to this issue.