Problem/Motivation

If the user enters a public or private document root on the Credential form that only contains slashes then all slashes are removed from the filepath resulting in an invalid path.
$path = str_replace($this->configuration['constants']['source_base_path'], NULL, $path);
Discovered while working on #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations

Proposed resolution

If the user input is only slashes then skip the str_replace.

Remaining tasks

Patch, add tests, review, commit

CommentFileSizeAuthor
#11 3160015-6.patch2.25 KBbenjifisher
#6 3160015-6.patch2.25 KBquietone
#6 interdiff-3-6.txt2.04 KBquietone
#3 3160015-2.patch2.32 KBquietone
#3 3160015-2-fail.patch1.4 KBquietone

Comments

quietone created an issue. See original summary.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Related issues: +#3151363: Double // in file paths

Perhaps #3151363: Double // in file paths is related. We might even combine the two issues.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 3160015-2-fail.patch1.4 KB
new 3160015-2.patch2.32 KB

And a patch with test. No interdiff because they are so small.

personal note:
It is with sadness that I could not use '/home/hypatia' (Hypatia) in the test because of the spellchecker. I found that 'ada' and 'barbara' was accepted but I wanted to recognize other women. So, I settled on '/home/lillian' for Lillian Moller Gilbreth.

Yes, I could have used 'foo'. But I value recognizing those who came before us.

What is most concerning to me is that the accepted names are very culture specific.

The last submitted patch, 3: 3160015-2-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs review » Needs work 
+++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
@@ -88,7 +88,9 @@ public function prepareRow(Row $row) {
-    $path = str_replace($this->configuration['constants']['source_base_path'], NULL, $path);
+    if (!preg_grep('#^/+$#', [$this->configuration['constants']['source_base_path']])) {
+      $path = str_replace($this->configuration['constants']['source_base_path'], NULL, $path);
+    }

I don't think we want to skip the string replace, I think we want preg_replace('/' . preg_quote($this->configuration['constants']['source_base_path']) . '/', '', $path, 1);? We still want to remove the first slash here.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3-6.txt2.04 KB
new 3160015-6.patch2.25 KB

Agree, that is better.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs review » Reviewed & tested by the community

Thanks! looks better!

  • larowlan committed 6e1d970 on 9.1.x 
    Issue #3160015 by quietone, mikelutz: Don't remove all slashes from...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Don't remove all slashes from filepath in file.php » [backport] Don't remove all slashes from filepath in file.php
Version: 9.1.x-dev » 9.0.x-dev
Issue tags: +Bug Smash Initiative

Committed 6e1d970 and pushed to 9.1.x. Thanks!

Flagging for possible backport

quietone’s picture

quietone commented

Retested with 9.0.x and tests still passing.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
StatusFileSize
new 3160015-6.patch2.25 KB

I am re-uploading the patch from #6 because it looks as though the tests are still running it against 9.1.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3160015-6.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure in Media_library.Drupal\Tests\media_library\FunctionalJavascript\EntityReferenceWidgetTest

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

+1 to backporting this.

  • larowlan committed d2edf3e on 9.0.x 
    Issue #3160015 by quietone, mikelutz: Don't remove all slashes from...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: [backport] Don't remove all slashes from filepath in file.php » Don't remove all slashes from filepath in file.php
Status: Reviewed & tested by the community » Fixed

backported to 9.0.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.