Ensure that migrations of entities which have file or image fields depend on private/public files migration

1. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +      catch (PluginException $e) {
   +        // Plugin cannot be instantiated.
   +      }
   

   🤔 Shouldn't this be \Drupal\Component\Plugin\Exception\PluginNotFoundException?

2. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +    // None of the plugins is a migrate field plugin instance.
   

   🤓 s/is/are/

3. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   +   *   The config factory service.
   

   🤓 Mismatch.

4. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +  public function addFileMigrationDependencies(array &$migrations, array $field_migration_plugin_ids) {
   

   This is a super long method. It would benefit a lot from splitting up into helper methods that are more easily understood.

5. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +    $field_plugin_classes = array_reduce($field_plugin_instances, function ($carry, $plugin_instance) {
   

   Nit: we could add typehints to the anonymous function.

6. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +      $destination_entity_type_id = in_array(explode(':', $destination_plugin)[0], $entity_destination_plugins, TRUE) && count(explode(':', $destination_plugin)) === 2 ?
   +        explode(':', $destination_plugin)[1] : NULL;
   

   🤓 Usually we format this as

   $r = $condition
     ? $if_true
     : $if_false;
   

7. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +      // Skip if the destination is not a content entity.
   +      if (!$definition instanceof ContentEntityTypeInterface) {
   +        continue;
   +      }
   

   Interesting — what are examples of when this edge case occurs?

8. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +        $destination_bundle = $migration_plugin['source']['node_type'] ?? $migration_plugin['source']['bundle'] ?? NULL;
   

   This is node-specific.

   What would it take to make this support bundles of other entity types? (It does already support arbitrary entity types that are bundleless 👍)

9. +++ b/core/modules/file/src/FileMigrationDependencyManager.php
   @@ -0,0 +1,203 @@
   +          // Add migration dependency metadata for public files.
   +          if ($file_field_scheme === 'public') {
   +            $file_migration_dependency_is_missing = array_search('d7_file', $dependencies) === FALSE;
   +
   +            if ($file_migration_dependency_is_missing) {
   +              $dependencies[] = 'd7_file';
   +            }
   +          }
   +          // Add migration dependency metadata for private file migration.
   +          elseif ($file_field_scheme === 'private') {
   +            $private_file_migration_dependency_is_missing = array_search('d7_file_private', $dependencies) === FALSE;
   +
   +            if ($private_file_migration_dependency_is_missing) {
   +              $dependencies[] = 'd7_file_private';
   +            }
   +          }
   

   😍 Very elegant!

10. +++ b/core/modules/file/src/FileMigrationDependencyManagerInterface.php
    @@ -0,0 +1,24 @@
    +interface FileMigrationDependencyManagerInterface {
    

    What would be a reason for somebody to want to provide an alternative implementation of this service?

+++ b/core/modules/file/src/FileMigrationDependencyManager.php
@@ -0,0 +1,203 @@
+      if (!isset($migration_plugin['migration_tags']) || !is_array($migration_plugin['migration_tags']) || !in_array('Drupal 7', $migration_plugin['migration_tags'], TRUE)) {

It is more reliable to get the source version number from the source database. As in migrate_drupal.module.

I guess I am missing something, why not just add the file migrations as optional dependencies ?

I guess I am missing something, why not just add the file migrations as optional dependencies ?

Back in early 2020 I would've said that is not possible, but now I agree it is!

+++ b/core/modules/file/src/FileMigrationDependencyManager.php
@@ -0,0 +1,203 @@
+      if (!empty($required_file_migration_plugins)) {
+        $migration_plugin['migration_dependencies']['required'] = array_unique(
+          array_merge(
+            $migration_plugin['migration_dependencies']['required'],
+            $required_file_migration_plugins
+          )
+        );

This logic (and related logic) should be updated to add these as optional instead of required dependencies.

Rerolled #4 for 9.3.x

Unable to provide complete interdiff file due to below error:

1 out of 1 hunk FAILED -- saving rejects to file /var/folders/1_/z9gwdk392dxdr23sq3cl1ryh0000gn/T//interdiff-1.6puoey.rej
interdiff: Error applying patch1 to reconstructed file

#12 looks identical to #4 — except for different whitespace in file.services.yml 👍

New edge case discovered when using https://www.drupal.org/project/file_entity. This updated patch adds protection for it.

Next patch can we change the name of it, unless it's not meant to be tested?

addFileMigrationDependencies
Can this be typehinted since it's a new function?

file.migration.dependency.manager:
As a new service believe this will need a change record.

The edge case you mentioned in #16 could that be added to the issue summary too please.

Thanks!

Trivial conflicts when applying #16 to 10.2.0:

• #3325557: Enable more service autowiring by adding interface aliases to core modules
• #3221793: Move file upload validation from file.module to constraint validators

Changing patch to MR to allow easier review.

@smustgrave - What you you mean by "...addFileMigrationDependencies... Can this be typehinted since it's a new function?"

The latest patch introduces a hard dependency on migrate module inside file.module. That is not really acceptable, as it can cause errors in install from config.

Fatal error: During class fetch: Uncaught ReflectionException: Class "Drupal\migrate\Plugin\MigrationDeriverTrait" not found while loading "Drupal\file\FileMigrationDependencyManager". in /var/www/html/vendor/composer/ClassLoader.php:576 Stack trace: #0 [internal function]: Composer\Autoload\ClassLoader->loadClass('Drupal\\file\\Fil...') #1 /var/www/html/vendor/symfony/config/Resource/ClassExistenceResource.php(76): class_exists('Drupal\\file\\Fil...') #2 /var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php(361): Symfony\Component\Config\Resource\ClassExistenceResource->isFresh(0) #3 /var/www/html/vendor/symfony/dependency-injection/Compiler/RegisterAutoconfigureAttributesPass.php(32): Symfony\Component\DependencyInjection\ContainerBuilder->getReflectionClass('Drupal\\file\\Fil...', false) #4 /var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php(80): Symfony\Component\DependencyInjection\Compiler\RegisterAutoconfigureAttributesPass->process(Object(Drupal\Core\DependencyInjection\ContainerBuilder)) #5 /var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php(767): Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(Drupal\Core\DependencyInjection\ContainerBuilder)) #6 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(1447): Symfony\Component\DependencyInjection\ContainerBuilder->compile() #7 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(971): Drupal\Core\DrupalKernel->compileContainer() #8 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(515): Drupal\Core\DrupalKernel->initializeContainer() #9 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(739): Drupal\Core\DrupalKernel->boot() #10 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #11 {main} in /var/www/html/web/core/modules/file/src/FileMigrationDependencyManager.php on line 18

The MigrationDeriverTrait only has one method.

Removing the Trait call and moving that method into FileMigrationDependencyManager.php fixes the issue, but I suspect this may affect all uses of the Trait.

It seems to be a race condition regarding how the plugin alter hook gets called.

UPDATE: I was testing an older patch version against Drupal 10.3. The newest patch no longer applies, so it is unclear whether the install problem is fixed.