when a module is installed, it may ship with default configuration.

right now, we just write that config to the active store, without allowing other modules to react.

for instance, if a module ships with a new field, we need to allow field module to react and make database changes etc.

so, this issue will create a new hook_config_module_default() to allow other modules to react.

#8 drupal-allow_modules_react_new_module_config-1496914-8.patch1.44 KBacrollet
PASSED: [[SimpleTest]]: [MySQL] 35,697 pass(es). View
#3 drupal-allow_modules_react_new_module_config-1496914-3.patch1.42 KBacrollet
PASSED: [[SimpleTest]]: [MySQL] 35,677 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


beejeebus’s picture

Issue tags: +Configuration system

add tag.

acrollet’s picture

Assigned: Unassigned » acrollet
acrollet’s picture

1.42 KB
PASSED: [[SimpleTest]]: [MySQL] 35,677 pass(es). View

Patch attached. It's worth noting that in its current configuration, if an installed module is disabled but not uninstalled, this hook will not fire when the module is re-enabled. Not sure if that use case should or shouldn't be accounted for.

acrollet’s picture

Status: Active » Needs review

issue status.

marcingy’s picture

Issue tags: +Needs tests
acrollet’s picture

Issue tags: -Needs tests

After a conversation with heyrocker and a search through the current tests by marcingy, it seems that

a) it may not be necessary to, in effect, test Drupal's hook system for a specific invocation, and
b) no current core modules are testing hook invocations in a generic manner.

Removing the 'needs tests' tag, but the final call is heyrocker's.

Lars Toomre’s picture

Can we add type hinting to the @param directives in config.api.php?

acrollet’s picture

1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 35,697 pass(es). View

Added type hinting and re-rolled.

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, drupal-allow_modules_react_new_module_config-1496914-8.patch, failed testing.

acrollet’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system
yched’s picture

I wonder if, from the point of view of a subsystem (field API, image styles...), this is really different from the "reload" operation. It's only about reacting to a new state of config files ?

I mean, a module would need to implement both hook_config_reload() and hook_config_module_default(), but at first sight I'd say that the code in hook_config_module_default() would just treat a degenerated case of hook_config_reload() where there are no "file changed" or "file removed" items, only "file added" items.

acrollet’s picture

Referencing #1447686: Allow importing and synchronizing configuration when files are updated so we don't have to find it again if this issue turns out to be a duplicate. It seems we may have a semantic issue here, namely, should hook_config_reload be fired at install time, when nothing's being "re"-loaded?

jhodgdon’s picture

Status: Needs review » Needs work

Please follow our documentation standards. For instance:

+ * @file
+ * Hooks provided by the Config module.
+ */

This should be Configuration Manager module (use the full name and correct capitalization).

And the hook docs:

+ * Allow modules to respond to a path being inserted.

What type of a path? Keep in mind that this is part of the "hooks" topic/group, so it would be better if this stood alone, something like "Allow modules to respond to a new configuration path.".

And can I suggest now, before this is all solidified, that "hook_config_module_default" is not a great name for this hook? Why does it have "module" in the name? That seems a bit odd... Try making a hook name that is more in line with other hooks in Drupal. Possibly better names I could think of (maybe you can pick one that is more descriptive of what it does):
hook_config_path_insert() [this fits with the one-line description -- if "default" is really important to the hook name, it should be part of the one-line description too?]

Also, comment #11 referenced a 'reload' hook. Does this exist? If so, it should be documented, and I would also suggest giving both hooks similar names and similar documentation, and using @see to cross-reference them.

acrollet’s picture

Status: Needs work » Postponed

@jhodgdon: please see #1447686: Allow importing and synchronizing configuration when files are updated for information about the reload operation. On the whole I think it's best to postpone this patch until that issue has landed, as it's entirely possible that this could be handled there.

jhodgdon’s picture

RE #16 - I don't really care about the reload operation. I just care that this patch doesn't go in with documentation that totally violates our documentation standards, or that introduces an API with inconsistent names that will just need to be cleaned up later.

acrollet’s picture

@jhodgdon: totally understood and agreed, and I'll re-roll according to your suggestions once #1447686: Allow importing and synchronizing configuration when files are updated has landed, assuming this issue will still be relevant.

sun’s picture