Fix config importing through the UI and change ConfigImportAll test using the UI

Batching was done as part of #1808248: Add a separate module install/uninstall step to the config import process :)

If you import configuration through the UI then modules are installed and uninstalled as part of batch through the BatchConfigImporter. Batching will not fix the time issues in the ConfigImportAll test since the process that runs the test will always take longer than any step.

That said I think we should re-purpose this issue to change the ConfigImportAll test since this reveals a number of issues:

1. The test does not work
2. One reason for this is you should be able to uninstall config through a config import in the UI since this will break the batch (lol)

Patch:

• moves BatchConfigImporter to the config module since it totally should be there and not Core\Config.
• Event listener to ensure that a BatchConfigImporter will not uninstall the config module (with a test)
• Changes the ConfigImportAllTest to use the batch - the fails we due to the local tasks being calculated on the batch page - implemented a hack to prove this - needs further discussion.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -281,6 +281,9 @@ public function getLocalTasksForRoute($route_name) {
   public function getTasksBuild($current_route_name) {
+    if ($current_route_name == 'system.batch_page.json') {
+      return array();
+    }
     $tree = $this->getLocalTasksForRoute($current_route_name);
     $build = array();

Here's the hack. Should we really be building a render array for local tasks on a batch json page?

There is no reason that batch pages should be adding local tasks and actions. @dawehner and I discussed this on IRC the easiest thing to do is the same we're doing for messages - have a killswitch. And this only occurs for batches that are not using javascript :) - for example all the batches running in simpletest.

The reason the route is missing is because we're doing lots of module enables without a route rebuild - I'd really like to avoid rebuilding the router after every module enable if at all possible.

moves BatchConfigImporter to the config module since it totally should be there and not Core\Config.

Wondering whether that means that config.module has to be installed, in order to import config via external tools/scripts (e.g. Drush)...?

AFAIK, we wanted to constrain the config module to be limited to a UI only; i.e., it should not be required for the API functionality. Is that still the case?

Once upon a time in #1833732-6: Find a way to skip ./config directories without skipping core/modules/config directory in drupal_system_listing(), there was a proposal to rename the module to config_ui.module to explicitly clarify on that front (consistent with other _ui modules; and doing so would allow us to remove a workaround from ExtensionDiscovery).

Lastly, I wonder, don't we need the batch functionality to be available sans config.module, in order to resolve things like #1613424: [META] Allow a site to be installed from existing configuration?

There is no reason that batch pages should be adding local tasks and actions. [...] the easiest thing to do is the same we're doing for messages - have a killswitch.

Hm. That existing #show_messages killswitch in the page rendering pipeline is actually some legacy code that is slated for removal... (can't remember which issue refactors it)

Unless I misunderstand the situation, the root cause actually asks for something different:

The import should actually enable maintenance mode for the duration of the import (disabling it upon successful completion), and the batch should operate on a maintenance page, also setting a MAINTENANCE_MODE constant, so that the maintenance page rendering process does not try to add arbitrary things (that rely on configuration) to the page.

In essence, pretty much following the update.php concept?

I'd really like to avoid rebuilding the router after every module enable if at all possible.

Hm. That is actively being discussed in #2201785: Remove drupal_flush_all_caches() from installer

The current lack of a router rebuild in ModuleHandler::install() appears to cause problems for contributed modules that are relying on route information in hook_install().

I guess it would make sense to hash out the discussion over there first?

@sun good point re the installer - so moving BatchConfigImporter back. Drush uses the plain ConfigImporter so no, the Config module does not need to be installed. But if we do want to be able to install from configuration sometime in the future then we're going to need this code.

If we have to create an script similar to update.php then we need to work out where to put this. Having a URL like /core/modules/config/config_sync.php is going to be ugly. But perhaps that will be the only way to truly isolate config synchronisations.

Hmmm... this no longer needs the theme hacks but it looks like we introduced more problems. With 100 database connections I get an error saying I've exceed them. With 200 I get other errors. Let's see what testbot says.

This patch adds a new flush step if modules are installed during the the import cycle. The step rebuilds the container and calls dfac(). The container rebuild is necessary to ensure that dfac() is using services based on the currently installed modules. This only became necessary after #2016629: Refactor bootstrap to better utilize the kernel.

What is interesting is now if I run ConfigImportAll test through the UI it passes, through run-tests.sh it fails due to too many db connections but if I turn xdebug on then it passes. Confused.

Bumping this to critical since this is exposing the fragility of the configuration importer.

Can someone test this through the UI and CLI (without xdebug enabled) locally and report what they find. If my max mysql connections are 200 it passes - if they are 10 it fails and sometimes it fails with 100.

Got it... the config database storage is causing additional unnecessary connections to be created when it is unserialised. This will pass (UI and CLI) even when you have only 10 mysql connections available.

Looks generally good. The comment on the flush step was a bit confusing for me, but then re-reading I got it. OTOH the migrate changes looks like misc bugfix while the simpletest change looks unrelated / unnecessary? No reasons were provided in #11 were they were introduced.

Here's why.

1. +++ b/core/modules/migrate/src/Entity/Migration.php
   @@ -79,6 +79,13 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
      /**
   +   * The configuration describing the load plugins.
   +   *
   +   * @var array
   +   */
   +  public $load;
   
   @@ -339,18 +346,4 @@ public function checkRequirements() {
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function toArray() {
   -    // @todo Remove once migration config entities have schema
   -    //   https://drupal.org/node/2183957.
   -    $class_info = new \ReflectionClass($this);
   -    foreach ($class_info->getProperties(\ReflectionProperty::IS_PUBLIC) as $property) {
   -      $name = $property->getName();
   -      $properties[$name] = $this->get($name);
   -    }
   -    return $properties;
   -  }
   

   Not all public properties on the class need to exported this code was leading to failures in the ConfigImportAll test due to the toArray adding new and unnecessary property load property that was dynamically defined. So I've added the property and changed migrate entities to use their schema (now they have one).

2. +++ b/core/modules/migrate/src/Entity/Migration.php
   --- a/core/modules/simpletest/src/Form/SimpletestResultsForm.php
   +++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
   
   +++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
   @@ -48,6 +48,12 @@ public static function create(ContainerInterface $container) {
      public function __construct(Connection $database) {
        $this->database = $database;
   +  }
   +
   +  /**
   +   * Builds the status image map.
   +   */
   +  protected function buildStatusImageMap() {
        // Initialize image mapping property.
        $image_pass = array(
          '#theme' => 'image',
   @@ -96,6 +102,7 @@ public function getFormId() {
   
   @@ -96,6 +102,7 @@ public function getFormId() {
       * {@inheritdoc}
       */
      public function buildForm(array $form, array &$form_state, $test_id = NULL) {
   +    $this->buildStatusImageMap();
   

   This change is necessary because the route's get rebuilt during an interactive configuration sync. During a route rebuild all the constructors get called. This constructor was calling through to the current user services in a cache set. This service is not available during the Kernel destruction phase.

Thanks for explaining, makes total sense, looks good to me :)

For more on the second problem in #15 see #2300131: EntityResolverManager instantiates objects unnecessarily - this would actually fix the issue but the less we do in a constructor the better so I don't think we should make this change dependent on that.

+ // rebuilt first the entity types are not discovered correctly due to using
+ // an entity manager that has the incorrect container namespaces injected.
+ \Drupal::service('kernel')->rebuildContainer(TRUE);

Do we need this? Can't we just $container->set('entity.manager', NULL); do we really need to flush all the caches and rebuild the container? drupal_flush_all_caches() has

  PhpStorageFactory::get('service_container')->deleteAll();
  \Drupal::service('kernel')->updateModules($module_handler->getModuleList(), $files);

do we really need to force a container rebuild ?

Yep we do. The problem is that dfac() has a call to system_rebuild_module_data() which eventually calls field_system_info_alter() which calls into the entity manager and this sets the entity type cache before the kernel has been rebuilt. Messy.

This ain't a task.

FYI: looks like this was retested (successfully) 4 hours ago (https://qa.drupal.org/pifr/test/818308), shortly after the RTBC in #16.

Patch looks good to me. The following question and docs nit don't need to block commit.

1. +++ b/core/modules/config/src/ConfigSubscriber.php
   @@ -0,0 +1,42 @@
   +      $importer->logError($this->t('Can not uninstall the Configuration module as part of a configuration synchronization through the user interface.'));
   

   Message says "through the user interface", but I don't see any if statement that limits this to that. Do validators in general not run for non-UI syncs?

2. +++ b/core/modules/config/src/Tests/ConfigImportUITest.php
   @@ -331,6 +331,21 @@ public function testImportValidation() {
   +  public function testConfigUninstallConfigException() {
   

   phpDoc?

1. We used to be able to tell I guess we could change this to a PHP_SAPI check.
2. Oops

As a followup, lets only validate when this function is FALSE - https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...

Good cleanup and great bug-catcher. Committed to 8.x. Thanks Alex.

Let's file a followup for #24.