Double // in file paths

The failure is typically caused by a missing file. It is not caused by the duplicated slash, but I see // in the logged message.

I am not sure that this is the right way to fix it, but the attached patch removes the . '/'. Let's see what the testbot has to say.

I've gone back and forth on this 3 times now. Just do it. There is so much wrong with that code block that is all being worked on in #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations. That particular line seems minor, but it's obvoiusly wrong to have a trailing slash there when the migration definitely appends one, and it's far better to change the core batch UI form to match the migration than to change the migration itself.

unrelated test failure

Do we not need test coverage for this one?

#10 Patch applied successfully.

Normally I do not review an issue where I have contributed a patch. In this case, my contribution (the non-test part of the patch) was approved in #6. I think it is OK for me to review the test.

1. I checked the test-only patch, and it fails in the expected way:

Failed asserting that ‘File’/var/www/html/core/modules/migrate_drupal_ui/tests/src/Functional/d7/files//sites/default/files/foo.txt’ does not exist’ contains "/migrate_drupal_ui/tests/src/Functional/d7/files/sites/default/files/foo.txt".

1. Do we need all this?

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/DoubleSlashTest.php
    @@ -0,0 +1,130 @@
    ...
    +    $default_db = Database::getConnection()->getKey();
    +    Database::setActiveConnection($this->sourceDatabase->getKey());
    +    $db = Database::getConnection($this->sourceDatabase->getKey());
    ...
    +    Database::setActiveConnection($default_db);

I think we just need the line that sets $db. Also, Database::setActiveConnection() returns the old value, so we could combine the first two lines if we did need them.

1.   +    $db = Database::getConnection($this->sourceDatabase->getKey());
     +
     +    $db->update('file_managed')
     +      ->condition('fid', 1)
     +      ->fields([
     +        'filename' => 'foo.txt',
     +        'uri' => 'public://foo.txt',
     +      ])
     +      ->execute();

Since $db is never used again, we could eliminate it. That would also remove the annoyance of the not-quite-aligned -> operators.

1.   +    // Run the upgrade.
     +    $this->drupalGet('/upgrade');
     +    $session = $this->assertSession();
     +
     +    // Get valid credentials.
     +    $edits = $this->translatePostValues($this->getCredentials());
     +
     +    // Start the upgrade process.
     +    $this->drupalGet('/upgrade');
     +    $session->responseContains("Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal $this->destinationSiteVersion.");
     +
     +    $this->submitForm([], t('Continue'));
     +    $session->pageTextContains('Provide credentials for the database of the Drupal site you want to upgrade.');
     +
     +    $this->submitForm($edits, t('Review upgrade'));
     +
     +    $this->submitForm([], t('I acknowledge I may lose data. Continue anyway.'));
     +    $session->statusCodeEquals(200);
     +
     +    $this->submitForm([], t('Perform upgrade'));

It looks as though this is mostly copied from IdConflictTest, and the assertions duplicate parts of that test. Can we get rid of the first drupalGet() and everything involving $session?

@quietone, thanks for the quick response!

I think the patch looks good. If the testbot disagrees, then it can overrule me. ;)

diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/d7/DoubleSlashTest.php b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/DoubleSlashTest.php
index 1533ef3488..d1972b529a 100644
--- a/core/modules/migrate_drupal_ui/tests/src/Functional/d7/DoubleSlashTest.php
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/DoubleSlashTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\migrate_drupal_ui\Functional\d7;
 
-use Drupal\Core\Database\Database;
 use Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeExecuteTestBase;
 use Drupal\migrate\Plugin\MigrationInterface;
 

Remvoed unused use on commit.

Oops. Thanks for catching that!