Followup to #2090115: Don't install a module when its default configuration has unmet dependencies

The config importer changes the source source used by the config installer so that configuration is read from the staging directory during module install during a configuration import. This is done by setting the storage on the config installer. The module installer then gets this and sets it again after every module enable because enabling modules rebuilds the container. Since the module installer is persisted through module installs the config importer sure set something on the module installer to use the alternative config storage and then we can simplify the config installer.

This is a bug because the config installer is re-entrant. If a language is created during an install then during the config save event the config installer is called to create any language config overrides.

Files: 
CommentFileSizeAuthor
#2 2451365.2.patch17.21 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,916 pass(es). View

Comments

alexpott’s picture

alexpott’s picture

Status: Active » Needs review
FileSize
17.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,916 pass(es). View

Patch attached completely removes the global state used to maintain the sync status in the ConfigInstaller during a module installation.

alexpott’s picture

+++ b/core/modules/system/module.api.php
@@ -160,12 +160,16 @@ function hook_modules_installed($modules) {
  * Please be sure that anything added or modified in this function that can
  * be removed during uninstall should be removed with hook_uninstall().
  *
+ * @param bool $in_config_sync
+ *   Whether or not the module installation is being perform as part of a
+ *   configuration synchronization. Defaults to FALSE.
+ *
  * @see hook_schema()
  * @see \Drupal\Core\Extension\ModuleHandler::install()
  * @see hook_uninstall()
  * @see hook_modules_installed()
  */
-function hook_install() {
+function hook_install($in_config_sync = FALSE) {

This is a pretty big change but I think it is extremely beneficial since it ensures that people writing hook_install() implementations should consider the fact that the code will run as part of a config sync.

mgifford’s picture

Status: Needs review » Needs work

Patch no longer applies.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

james.williams’s picture

I've just ran into an issue (#2852607-6: Error when importing core.extension.yml) where we discovered the ModuleInstaller was calling ConfigInstaller's public setSourceStorage method, which isn't in the ConfigInstallerInterface ... so I'd be in favour of this issue as it removes that problem too.
(Otherwise I'd create a ticket about adding setSourceStorage to the interface, but I think this wider issue around the global syncing state is more important.)

james.williams’s picture

I just started progress on a re-roll, but then realised that other parts of code still need to know about the syncing state. For example, see #2711645: ConfigInstaller::isSyncing() should return true always during a config sync - for that ticket, commerce needed to detect whether a config sync was in process during hook_entity_bundle_create(), which I don't think would be possible with this change?