Problem/Motivation

Inside \Drupal\image\Plugin\Field\FieldType\ImageItem::generateSampleValue() we currently have:

        (...)
        $destination_dir = static::doGetUploadLocation($settings);
        file_prepare_directory($destination_dir, FILE_CREATE_DIRECTORY);
        $destination = $destination_dir . '/' . basename($path);
        $file = file_move($image, $destination, FILE_CREATE_DIRECTORY);
        (...)

However, the constants accepted for file_move() are:

 * @param int $replace
 *   (optional) The replace behavior when the destination file already exists.
 *   Possible values include:
 *   - FILE_EXISTS_REPLACE: Replace the existing file. If a managed file with
 *     the destination name exists then its database entry will be updated and
 *     $source->delete() called after invoking hook_file_move(). If no database
 *     entry is found, then the source files record will be updated.
 *   - FILE_EXISTS_RENAME: (default) Append _{incrementing number} until the
 *     filename is unique.
 *   - FILE_EXISTS_ERROR: Do nothing and return FALSE.

Proposed resolution

Fix the wrong constant.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 2931709-8.4.x-14.patch1.47 KBmarcoscano
#6 interdiff-4-6.txt1.49 KBmarcoscano
#6 2931709-6.patch1.49 KBmarcoscano
#4 interdiff-2-4.txt1.53 KBmarcoscano
#4 2931709-4.patch1.53 KBmarcoscano
#2 2931709-2.patch858 bytesmarcoscano

Comments

marcoscano created an issue. See original summary.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Assigned: marcoscano » Unassigned
Status: Active » Needs review
StatusFileSize
new 2931709-2.patch858 bytes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Needs work 
+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -354,7 +354,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+        $file = file_move($image, $destination, FILE_EXISTS_REPLACE);

Good catch, but why FILE_EXISTS_REPLACE? I think we don't want to replace an (unlikely) existing file. Let's go with the default, which is FILE_EXISTS_RENAME.

Also, I see the same bug few lines above:

file_unmanaged_move($tmp_file, $destination, FILE_CREATE_DIRECTORY);

Let's fix that too.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs work » Needs review
StatusFileSize
new 2931709-4.patch1.53 KB
new interdiff-2-4.txt1.53 KB

Good catch, but why FILE_EXISTS_REPLACE?

Current constant values are:

/**
 * Flag used by file_prepare_directory() -- create directory if not present.
 */
const FILE_CREATE_DIRECTORY = 1;

/**
 * Flag used by file_prepare_directory() -- file permissions may be changed.
 */
const FILE_MODIFY_PERMISSIONS = 2;

/**
 * Flag for dealing with existing files: Appends number until name is unique.
 */
const FILE_EXISTS_RENAME = 0;

/**
 * Flag for dealing with existing files: Replace the existing file.
 */
const FILE_EXISTS_REPLACE = 1;

/**
 * Flag for dealing with existing files: Do nothing and return FALSE.
 */
const FILE_EXISTS_ERROR = 2;

So by (wrongly) using FILE_CREATE_DIRECTORY, we are currently already using FILE_EXISTS_REPLACE (value: 1). I was assuming the safest was not to change the actual behavior of the code, but it's true that renaming makes more sense than replacing here.

Thanks!

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Needs work 
+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -344,7 +344,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+      file_unmanaged_move($tmp_file, $destination, FILE_EXISTS_RENAME);

@@ -354,7 +354,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+        $file = file_move($image, $destination, FILE_EXISTS_RENAME);

Thank you. As FILE_EXISTS_RENAME is the default value for that parameter, we don't need to pass it explicitly, right? :)

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs work » Needs review
StatusFileSize
new 2931709-6.patch1.49 KB
new interdiff-4-6.txt1.49 KB

🤦

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

RTBC if turns green.

The last submitted patch, 4: 2931709-4.patch, failed testing. View results

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

APCU failure.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Adding review credits for @claudiu.cristea

  • larowlan committed 6585630 on 8.5.x 
    Issue #2931709 by marcoscano, claudiu.cristea: Wrong constant name in \...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 6585630 and pushed to 8.5.x.

This can be backported to 8.4.x but doesn't apply

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs reroll
marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Patch (to be ported) » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2931709-8.4.x-14.patch1.47 KB

Re-rolled against 8.4.x

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC for 8.4.x

  • xjm committed 7d7340d on 8.4.x 
    Issue #2931709 by marcoscano, claudiu.cristea, larowlan: Wrong constant...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed the backport to 8.4.x as well. Thanks!

  • xjm committed 97bfb56 on 8.4.x 
    Revert "Issue #2931709 by marcoscano, claudiu.cristea, larowlan: Wrong...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.4.x-dev » 8.5.x-dev

Sorry, I spaced out and forgot that we're past the last normal patch release of 8.4.x, so marking this fixed against 8.5.x instead. :)

Status: Fixed » Closed (fixed)

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