Single configuration import must run dependencies check before the actual import execution

+++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
@@ -24,6 +24,30 @@
   /**
+   * Returns the entity id of the configuration object.
+   *
+   * @param string $name
+   *   The configuration object name.
+   * @param string $entity_type_id
+   *   The configuration object entity type id.
+   *
+   * @return string|null
+   *   Either the entity id, or NULL if none match
+   */
+  public function getEntityIdByEntityTypeId($name, $entity_type_id);
+
+  /**
+   * Returns the entity type id and entity id of the configuration object.
+   *
+   * @param string $name
+   *   The configuration object name.
+   *
+   * @return array
+   *   An array containing entity type id and entity id if something match.
+   */
+  public function getEntityTypeIdAndEntityIdByName($name);

How about a getEntityByConfigName($name) that just returns the loaded config entity? I'm not a huge fan of functions the return an array

These are mostly nitpicks about code style and documentation. The actual code looks pretty good!

1. Can you provide PHPUnit tests for the methods you added to ConfigManager? This ensures we have good code coverage.
2. When posting new patches in an issue, we generally post interdiffs (except for re-rolls). These are patches/diffs that describe what changed since the last patch. They allow reviewers to quickly look at only the code that changed, which saves time. For a tutorial on how to create interdiffs, see Creating an interdiff.

3. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -86,6 +86,20 @@ public function getEntityTypeIdByName($name) {
   +          return entity_load($entity_type_id, $id);
   

   Instead of using entity_load() (which prevents unit testing), use $this->entityManager->getStorageController()->load(). This is an injected dependency and can therefore be mocked in PHPUnit tests.

4. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -183,30 +197,92 @@ public function findConfigEntityDependents($type, array $names) {
   +    $return = TRUE;
   

   Debugging code?

5. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -183,30 +197,92 @@ public function findConfigEntityDependents($type, array $names) {
   +   * Validates modules existence.
   

   Checks if modules exist. This is probably easier to understand, and does not contain the grammatical error.

6. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -183,30 +197,92 @@ public function findConfigEntityDependents($type, array $names) {
   +   * @param array $names
   

   @param string[] to indicate this is an array of strings.

7. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -183,30 +197,92 @@ public function findConfigEntityDependents($type, array $names) {
   +   * Validates themes existence.
   

   Checks if themes exist. This is probably easier to understand, and does not contain the grammatical error.

8. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -183,30 +197,92 @@ public function findConfigEntityDependents($type, array $names) {
   +   * @param array $names
   

   @param string[]

9. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -183,30 +197,92 @@ public function findConfigEntityDependents($type, array $names) {
   +   * Validates configuration entities existence.
   

   Checks if configuration entities exist. This is probably easier to understand, and does not contain the grammatical error.

10. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -183,30 +197,92 @@ public function findConfigEntityDependents($type, array $names) {
    +   * @param array $names
    

    @param string[]

11. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -24,6 +24,17 @@
    +   *   Returns configuration entity object or NULL if entity doesn't exist.
    

    ...if the entity...

12. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -98,5 +109,16 @@ public function findConfigEntityDependents($type, array $names);
    +   * Validates existence of all given dependencies.
    

    ...the existence...

13. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -98,5 +109,16 @@ public function findConfigEntityDependents($type, array $names);
    +   * @param array $dependencies
    

    @param array[] to indicate this is an array of arrays. Please also extend the description with the structure of the child arrays.

14. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSingleImportForm.php
    @@ -177,6 +189,13 @@ public function validateForm(array &$form, array &$form_state) {
    +        $this->setFormError('import', $form_state, $this->t('Missing dependencies have been found. Please import them first.'));
    

    Our UI text standards stipulate that we do not say please, because we are not requesting users to do anything. Instead, we are simply telling them what happened and must be done to solve the problem.

1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -209,29 +209,36 @@ public function findConfigEntityDependentsAsEntities($type, array $names) {
   +          }
   

   Is there a reason why you are not returning the return value of $this->validateConfigEntityDependentModulesExistence() directly?

2. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
   @@ -110,14 +110,18 @@ public function findConfigEntityDependents($type, array $names);
   +   *   dimension array should contain a list of full configuration object names.
   

   Good! :)

Adding tag.

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -183,30 +197,99 @@ public function findConfigEntityDependents($type, array $names) {
+    }

This can be made simpler and faster by simply returning $this->validateConfigEntityDependentModulesExistence() && $this->validateConfigEntityDependentThemesExistence() && $this->validateConfigEntityDependentEntitiesExistence(). The fact that you added a separate method per dependency type is very good, and you should keep this :)

23: drupal-single_configuration_import_dependency_check-2224571-23.patch queued for re-testing.

Thanks much for working on this! A couple small things:

    $themes = \Drupal::service('theme_handler')->listInfo();
    foreach ($names as $theme_name) {
      if (!isset($themes[$theme_name])) {
        return FALSE;
      }
    }
    return TRUE;

Surely there's a handy array_diff or array_intersection that can do this without a userspace loop (sorry, 2am, can't think clearly which one)? There aren't enough themes usually that it'd be worth the eager return the userspace loop provides.

      $entity = $this->getEntityByConfigName($config_name);
      if (empty($entity)) {

Why not just if (!$this->getEntityByConfigName($config_name)) , why the separate variable, it's not needed as far as i can see. Same pattern as validateConfigEntityDependentModulesExistence.

By the way, both validateConfigEntityDependentModulesExistence and can be converted into the following pattern:

while (($result = $this->getEntityByConfigName(array_pop($names))) && $names);
return $result;

if all them exist then $result will remain TRUE (-ish) and $names will become empty and thus terminate the loop; the first time one doesn't exist $result is FALSE and the loop is immediately terminated. But that's perhaps a bit too dense :) ask the people around you at DrupalCon what they think :)

This is basically why the single import is problematic.

I'm concerned that we are adding a dependency on the module handler here - can we have separate validation code?

Did not check the patch yet, but we have the same problem for default configuration, we don't validate those either. Does this already fix it and if not, can we fix that here as well?

As an example, add a entity_view_display entity for a non-existing view mode as default configuration to a module. Then try to install that module => fatal error (that doesn't tell you what is broken and why). We should instead throw an exception or something that includes the config and which dependency is missing.

This is a problem in the config manager module, right?

We just need to ensure that all the validators we implement #1842956: [Meta] Implement event listeners to validate imports are run - especially #2416109: Validate configuration dependencies before importing configuration

Reroll.

This is a duplicate of #2486467: Single config import form needs to use the config validation events - which has a different approach but the same outcome.

@Taran2L can you comment on #2486467: Single config import form needs to use the config validation events so that we can give you credit for the work done on this issue?