When a file copy fails, the error message that is logged is very limited in that it doesn't give the destination path that is causing the issue.

File: core/lib/Drupal/Core/File/FileSystem.php

Current version

// Perhaps $destination is a dir/file?
$dirname = $this->dirname($destination);
if (!$this->prepareDirectory($dirname)) {
  $this->logger->error("The specified file '%original_source' could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions.", [
    '%original_source' => $original_source
  ]);
  throw new DirectoryNotReadyException("The specified file '$original_source' could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions.");
}

Example log output: The specified file 'temporary://filehDDLEO' could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions.

Proposed change

// Perhaps $destination is a dir/file?
$dirname = $this->dirname($destination);
if (!$this->prepareDirectory($dirname)) {
  $this->logger->error("The specified file '%original_source' could not be copied because the destination directory '%destination_dir' is not properly configured. This may be caused by a problem with file or directory permissions.", [
    '%original_source' => $original_source,
    '%destination_dir' => $dirname
  ]);
  throw new DirectoryNotReadyException("The specified file '$original_source' could not be copied because the destination directory '$dirname' is not properly configured. This may be caused by a problem with file or directory permissions.");
}

Example log output: The specified file 'temporary://filehDDLEO' could not be copied because the destination directory 'public://aaa/bbb' is not properly configured. This may be caused by a problem with file or directory permissions.

CommentFileSizeAuthor
#6 3070902-better-logging-for-preparedestination-exception-6.patch1.51 KBdalin
#19 interdiff-3070902-12-19.txt1.1 KBmrinalini9
#8 3070902-better-logging-for-preparedestination-exception-7.patch2.67 KBdalin
#19 better-logging-for-preparedestination-exception-3070902-19.patch2.68 KBmrinalini9
#12 better-logging-for-preparedestination-exception-3070902-12.patch2.68 KBmrinalini9
#12 interdiff-3070902-7-12.txt1.44 KBmrinalini9
#5 3070902-better-logging-for-preparedestination-exception-5.patch1.5 KBdalin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrupsidown created an issue. See original summary.

mrupsidown’s picture

mrupsidown CreditAttribution: mrupsidown commented
Issue summary: View changes
mrupsidown’s picture

mrupsidown CreditAttribution: mrupsidown commented
Issue summary: View changes
digitaldonkey’s picture

digitaldonkey CreditAttribution: digitaldonkey commented
Related issues: +#3077951: Directory "private:" is created if file_private_path is not set

Please https://www.drupal.org/project/drupal/issues/3070902

dalin’s picture

dalin
he/him
Primary language English
Location 🇲🇽, 🇨🇦, 🌍
CreditAttribution: dalin at Advomatic for Columbia University commented
Title: Files prepareDestination Exception improvement » Better logging for prepareDestination Exception
Version: 8.7.5 » 9.1.x-dev
Status: Active » Needs review
FileSize
3070902-better-logging-for-preparedestination-exception-5.patch1.5 KB
dalin’s picture

dalin
he/him
Primary language English
Location 🇲🇽, 🇨🇦, 🌍
CreditAttribution: dalin at Advomatic for Columbia University commented
FileSize
3070902-better-logging-for-preparedestination-exception-6.patch1.51 KB

Whoops, created the patch from the wrong directory. Here's another go.

Status: Needs review » Needs work

The last submitted patch, 6: 3070902-better-logging-for-preparedestination-exception-6.patch, failed testing. View results

dalin’s picture

dalin
he/him
Primary language English
Location 🇲🇽, 🇨🇦, 🌍
CreditAttribution: dalin as a volunteer and at Advomatic for Columbia University commented
FileSize
3070902-better-logging-for-preparedestination-exception-7.patch2.67 KB

Now with (hopefully) a passing test.

Bunty Badgujar’s picture

Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review

Moving to Need review as last submitted patch passes test cases.

Bunty Badgujar’s picture

Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs review » Needs work

After quick look to patch #8

Example output doesn't match with Patch. Following changes needed :- 

+        $this->logger->error("The specified file '%original_source' could not be copied because the destination directory %destination_directory is not properly configured. This may be caused by a problem with file or directory permissions.", [
           '%original_source' => $original_source,
+          '%destination_directory' => $dirname,
         ]);

should change to 

+        $this->logger->error("The specified file '%original_source' could not be copied because the destination directory '%destination_directory' is not properly configured. This may be caused by a problem with file or directory permissions.", [
           '%original_source' => $original_source,
+          '%destination_directory' => $dirname,
         ]);

and

+ throw new DirectoryNotReadyException("The specified file '$original_source' could not be copied because the destination directory $dirname is not properly configured. This may be caused by a problem with file or directory permissions.");

should change to
+ throw new DirectoryNotReadyException("The specified file '$original_source' could not be copied because the destination directory '$dirname' is not properly configured. This may be caused by a problem with file or directory permissions.");

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » mrinalini9
mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
better-logging-for-preparedestination-exception-3070902-12.patch2.68 KB
interdiff-3070902-7-12.txt1.44 KB

Updated patch #8 as per the changes mentioned in #10, please review.

Status: Needs review » Needs work

The last submitted patch, 12: better-logging-for-preparedestination-exception-3070902-12.patch, failed testing. View results

kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commented
Assigned: Unassigned » kishor_kolekar
kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commented
Assigned: kishor_kolekar » Unassigned

#8 patch looks good to me.
in my opnion @Bunty Badgujar I don't think we should not use string concatenation in $this->logger->error

kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commented
Status: Needs work » Needs review
Bunty Badgujar’s picture

Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs review » Needs work

Thanks @kishor kolekar for review. Its not string concatenation, I am talking about $dirname should be prefix and suffix with ' (aapostrophe). See issue discription.
Moving back to Need work as last submitted patch failed.

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » mrinalini9
mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
better-logging-for-preparedestination-exception-3070902-19.patch2.68 KB
interdiff-3070902-12-19.txt1.1 KB

Fixed test case failure issue in #12, please review.

dalin’s picture

dalin
he/him
Primary language English
Location 🇲🇽, 🇨🇦, 🌍
CreditAttribution: dalin as a volunteer and at Advomatic for Columbia University commented
Status: Needs review » Reviewed & tested by the community
dalin’s picture

dalin
he/him
Primary language English
Location 🇲🇽, 🇨🇦, 🌍
CreditAttribution: dalin as a volunteer and at Advomatic for Columbia University commented

The last submitted patch, 19: better-logging-for-preparedestination-exception-3070902-19.patch, failed testing. View results

  • catch committed 264d89c on 9.1.x 
    Issue #3070902 by dalin, mrinalini9, mrupsidown, Bunty Badgujar: Better...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Committed 264d89c and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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