Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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;
}
Comment | File | Size | Author |
---|---|---|---|
#24 | interdiff.txt | 3.89 KB | tim.plunkett |
#23 | config-import-hooks-1806178-23.patch | 22.3 KB | tim.plunkett |
#17 | interdiff.txt | 7.32 KB | tim.plunkett |
#17 | die-import-hooks-1806178-14.patch | 23.02 KB | tim.plunkett |
#14 | die-import-hooks-1806178-14.patch | 21.12 KB | tim.plunkett |
Comments
Comment #1
sunThe 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.
Comment #2
gddOne 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
Comment #3
sunWe don't need to "guess", because the config prefixes are precisely defined in entity plugin info/metadata.
Comment #4
gddBut 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?
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedyeah, 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.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedtagging.
Comment #7
gddCalling $style->delete() will trigger hook_ENTITY_TYPE_delete() so image_style_delete() gets called through that mechanism.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedre #7 - huh, ok then i don't know what image_config_import_delete() is doing then...
Comment #9
sun@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)
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedFrom beejeebus in #5:
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.
Comment #11
sun@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.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedOK, 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.
Comment #13
tim.plunkettComment #14
tim.plunkettHow about something along these lines?
Still needs some love, but it's worth a testbot run.
Comment #15
tim.plunkettObviously 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
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...
Comment #17
tim.plunkettIt 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))
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentednice 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.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedyay
Comment #20
andypost+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
Comment #21
tim.plunkettThis blocks conversions to ConfigEntity, so bumping.
Comment #22
sunBy 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:
Can we rename this to
config_get_entity_type_by_name()
?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.
This part of the phpDoc comments on the new/moved methods can be dropped, I think.
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).Can we keep + move this @todo, please? It's still one of the few remaining major unresolved tasks in the config entity system.
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.
Humm... there's no hook_config_import_change() for views...? Odd. ;)
Comment #23
tim.plunkettYes, 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)
Comment #24
tim.plunkettForgot the interdiff
Comment #25
sunThanks!
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.
Comment #26
tim.plunkettEvery 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.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedyeah, i agree with #26, lets deal with the config prefix stuff in another issue. changes look good, back to RTBC.
Comment #28
webchickHm. Is that robust enough? I suppose so, since the ops are a known list.
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!
Comment #29
gddSpawned #1886478: Bring back hook_config_import_CRUD() hooks as a followup to this issue.
Comment #30
alexpottSpawned #1889752: Remove unnecessary manifest creation in config_install_default_config() as a follow up to this issue.