Problem/Motivation

ConfigImportAllTest should use the UI to do the import because this is a better test of enabling modules and importing configuration and ensure the batch still works.

This is critical because config importing through the UI can be proved not to currently work.

Proposed resolution

Use Config UI.

Remaining tasks

Review and commit patch. The current patch in #13 contains fixes for various issue found whilst debugging config import failures:

  • Serialisation of the config DatabaseStorage caused additional database connections to be created during the sync process
  • A full container rebuild and cache flush is necessary after installing and uninstalling modules
  • A new config import validation event prevents the config module from being uninstalled so that the route will still exist after importing
  • Migrate config entities fixes to use their schema for deciding what properties to export
  • SimpletestResultsForm is constructed during a route rebuild this causes a failure because necessary services are not available during the kernel destruct event.

User interface changes

None.

API changes

None

N.b. this issue was re-purposed from the original issue because batching actually was implemented by #1808248: Add a separate module install/uninstall step to the config import process.

Original report by @YesCT

Problem/Motivation

#98 in #1808248: Add a separate module install/uninstall step to the config import process @alexpott

implement sensible batching of extension install and uninstall during config import.

1808248 introduced a test that enabled all modules at once. (explain why that is bad. something to do with loading all default configuration, and taking a really long time)

Files: 
CommentFileSizeAuthor
#13 2234159.13.patch8.14 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,607 pass(es). View
#13 11-13-interdiff.txt527 bytesalexpott
#11 2234159.11.patch7.51 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,038 pass(es). View
#11 9-11-interdiff.txt4.69 KBalexpott
#9 2234159.9.patch4.62 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,852 pass(es), 9 fail(s), and 4 exception(s). View
#8 2234159.8.patch10.67 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,733 pass(es). View
#8 5-8-interdiff.txt1.95 KBalexpott
#5 2234159.5.patch11.42 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,649 pass(es). View
#3 2234159.3.patch8.93 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,697 pass(es). View
#3 1-3-interdiff.txt6.57 KBalexpott
#1 2234159.1.patch1.44 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,446 pass(es), 54 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

Title: implement sensible batching of extension install and uninstall during config import. Do not enable all modules at once. » Change ConfigImportAll test to import through the UI
Status: Active » Needs review
FileSize
1.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,446 pass(es), 54 fail(s), and 0 exception(s). View

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)

Status: Needs review » Needs work

The last submitted patch, 1: 2234159.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.57 KB
8.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,697 pass(es). View

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.
alexpott’s picture

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

alexpott’s picture

FileSize
3.16 KB
11.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,649 pass(es). View

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.

alexpott’s picture

Issue summary: View changes
sun’s picture

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

alexpott’s picture

FileSize
1.95 KB
10.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,733 pass(es). View

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

alexpott’s picture

FileSize
4.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,852 pass(es), 9 fail(s), and 4 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 9: 2234159.9.patch, failed testing.

alexpott’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
4.69 KB
7.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,038 pass(es). View

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.

alexpott’s picture

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.

alexpott’s picture

FileSize
527 bytes
8.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,607 pass(es). View

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.

Gábor Hojtsy’s picture

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.

alexpott’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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.

chx’s picture

+ // 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 ?

alexpott’s picture

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.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Title: Change ConfigImportAll test to import through the UI » Fix config importing through the UI and change ConfigImportAll test using the UI
Category: Task » Bug report

This ain't a task.

effulgentsia’s picture

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

effulgentsia’s picture

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?

alexpott’s picture

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

moshe weitzman’s picture

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

  • Dries committed 4baeb8d on 8.x
    Issue #2234159 by alexpott: Fixed config importing through the UI and...
Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

xjm’s picture

Issue tags: +Needs followup

Let's file a followup for #24.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.