Specify table alias for File migration source plugin getIds method

I did literally exactly the same thing and encountered the same issue, so it's a huge +1 to have this fixed.

However, we should extend the test coverage to show the problem (publish a patch with test only) and then a fix to prove that it was fixed (patch with test + fix).

I'm also wondering if it makes sense to add a configurable filter by used files directly to the File source plugin. It might look like this:

if ($this->configuration['used_only']) {
  $query->innerJoin('file_usage', 'fu', 'f.fid = fu.fid');
}

I think it might be a good improvement but would like to get another opinion before creating a task.

In the first patch (and the first commit in merge request) I added a new test module with a custom plugin that extends File source plugin and joins file_usage table. (this is how we both reproduced this issue).

In the second patch there is the same test and fix from #2.

The request is ready for review, but there are still open questions:

1) As we can see, the test failed with the following message:

PHP Fatal error: Uncaught LogicException: The database connection is not serializable.

When I was debugging this on my local environment, I was able to see the actual exception message, which was the same as in the task description:

Migration failed with source plugin exception: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'fid' in on clause is ambiguous

I think it should be fixed (probably in a follow-up issue).

2) Do we want to include a configuration to include used files only in the original File source plugin?

I have reviewed the patch and here are my thoughts.

First, looks good and the test fixes the problem in the IS. it is disappointing to have to write a test module and a Migration test to prove this fixes the error reported in the IS. That said, the test does the job.

I found the mix of names used in the test module and the migration test a bit confusing. I think if we use 'file_used' or FileUsed it will make this easier to follow and maintain. That means only the two changes below need to be made.

• file_test_migration module -> migrate_file_used
• MigrateFileCustomSourcePluginTest.php -> MigrateFileUsedTest.php

1. +++ b/core/modules/file/tests/modules/file_test_migration/src/Plugin/migrate/source/d7/FileUsed.php
   @@ -0,0 +1,30 @@
   + * Drupal 7 file source from database restricted to used files only.
   

   nit, 'only' can be dropped without losing meaning.

2. +++ b/core/modules/file/tests/modules/file_test_migration/src/Plugin/migrate/source/d7/FileUsed.php
   @@ -0,0 +1,30 @@
   +    // Join on file_usage table to only migrate files used through certain
   +    // modules.
   

   This join isn't joining on the module name so the 'through certain modules.' can be removed.

3. +++ b/core/modules/file/tests/src/Kernel/Migrate/d7/MigrateFileCustomSourcePluginTest.php
   @@ -0,0 +1,26 @@
   + * Migrates used files in the file_managed table.
   

   This is testing the migration. So 'Tests the migration of used files.'

9.1 Yes, that error is a pain. Migrations never used to fail with the serialization error, it is a fairly recent nuisance.
9.2 I would say no, since it is not needed to migrate core. But it is still a good idea to ask the other maintainers.

@quietone, thanks for the review!

It was addressed in the last commit except this change: file_test_migration module -> migrate_file_used
The idea was to create a test module that might be re-used in the future (for any other migration modifications), so I didn't want to name it specifically for this plugin. Please let me know what you think about it.

@Matroskeen, your welcome!

And to my thoughtss...

My experience with migrate is that test modules are unique to a particular test. If migrations and/or plugins are added to this new test module sometime in the future then that changes the environment of the existing test and that should not happen. For example, if the current test included an assertion on the number of migrations discovered it would fail when a new migration was added. I know that is not the best example, but it is the principle about changing the test environment of an existing test to accommodate a different test.

And from a DX viewpoint, it is really handy to have the names in agreement. And if, say a new test migration was added to support a different test, and I was looking at the original test I would be confused why there is a second migration that is not used in the test. I would then spend time trying to find out why.

I hope that makes sense. ;-)

It does make sense, but I still have mixed feelings about the test and test module. I'll try to explain why.

We already changed the test name to MigrateFileUsedTest.php and almost changed the test module name to migrate_file_used. However, we do not test the actual filter by used files here. We do not care whether only used files were migrated. What we really do here - make sure the query is not broken when we join another table with the same column names. I think it might be even more confusing for other developers.

Another thing is the issue might exist in other migrate source plugins. Here is an example: #2987832: Term Node query Integrity constraint violation.
Following this approach, we'll have to create a separate test module and extend the original query for each source plugin, which sounds like overkill. 🤷‍♂️

Right, so your thinking this is just one example of adding aliases to getids() and that there will be other cases. That would change the scope here or we need a meta to investigate the need to do that and track progress for the existing 121 source plugins. We should bring that up at a migrate meeting.

However, I still think that the names should be consistent with the purpose of one test. And as you point out the test is really about getids and not files uses do the names would more accurately names test_file_getIds, MigrateFileGetIdsTest.

I was looking for better options (trying to reduce the number of changes or make the tests more generic) but didn't find any 😔
Published one more commit to address feedback in #14.

.

@Matroskeen, thank you for those changes!

I reviewed the patch and it fixes the problem in the IS and tests it is well. All good to go.

I wonder if this needs a change record?

@quietone we'd need a change record if we expect people to have to make changes to existing migrations - is that the case here?

@alexpott, existing migrations are fine. This helps anyone extending the file plugin.

Committed and pushed 118b1c33a7 to 9.2.x and b999b138a8 to 9.1.x. Thanks!