Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This is a follow up to #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations.
benjifisher reported
When I run file migrations and there are missing files, the message often lists a path including //, and I think one of those characters comes from this line. Do we need to add a trailing / here?
$configuration['source']['constants']['source_base_path'] = rtrim($base_path, '/') . '/';
Proposed resolution
TBD
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#17 | 3151363-17.patch | 3.89 KB | quietone |
#17 | interdiff-10-17.txt | 1.87 KB | quietone |
#10 | 3151363-10.patch | 4.54 KB | quietone |
#10 | 3151363-10-fail.patch | 3.64 KB | quietone |
#3 | 3151363-3.patch | 923 bytes | benjifisher |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedDo you recall the reason for the failure? I would be surprised if it was because of the double slash.
And d7_file, d6_file and d7_file_private migrations add a slash.
Comment #3
benjifisherThe 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.Comment #4
mikelutzI'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.
Comment #6
mikelutzunrelated test failure
Comment #7
catchDo we not need test coverage for this one?
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedIn my opinion specific tests are not needed. The fact that both the Upgrade6Test and Upgrade7Test pass is sufficient in my mind.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedWell, I suppose it can be tested like this.
Comment #12
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedComment #13
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commented#10 Patch applied successfully.
Comment #14
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedComment #15
quietone CreditAttribution: quietone as a volunteer commented@asishsajeev, thank you for setting this patch to RTBC, but having a patch apply successfully is only one of the criteria a patch must met in order to be RTBC. In this case, the patch in #10 adds a new test which needs to be reviewed. If you did that please add a comment explaining what you did to review the test. It takes a while to know the whole process and there is general information about reviews are in the handbook, there is a page about reviewing patches and there is a contributor page about patch review. Thx!
Comment #16
benjifisherNormally 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.
Do we need all this?
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.Since
$db
is never used again, we could eliminate it. That would also remove the annoyance of the not-quite-aligned->
operators.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
?Comment #17
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, thanks.
Yes this was a copy/paste from another test file and I meant to come back and clean it up. Clearly I forgot. Sorry.
Comment #18
benjifisher@quietone, thanks for the quick response!
I think the patch looks good. If the testbot disagrees, then it can overrule me. ;)
Comment #19
alexpottRemvoed unused use on commit.
Comment #21
benjifisherOops. Thanks for catching that!