Cms: setContentFromPost called from entries.onBeforeSaveEntry results in lost/empty $_FILES

[timkelty]

Description

I have a Front End entries/saveEntry form with a file upload.
When I submit with a file, I get the error:

The file “assets59d4f275a82c13.29118020.jpg” does not appear to be an image.

And indeed, if I look in /craft/storage/runtime/temp the file is either missing entirely, or there and empty. If I log the $mimeType, it is inode/x-empty.

I discovered what is causing this is a call to setContentFromPost from entries.onBeforeSaveEntry.

craft()->on('entries.onBeforeSaveEntry', function(Event $event) {
    $entry = $event->params['entry'];
    $isNewEntry = $event->params['isNewEntry'];

        // doesn't matter what is passed here…
        // if it is called at all, the file upload error occurs 
    $entry->setContentFromPost([]);
});

Note - other issues with a similar message seem to be related to drive storage and/or /tmp storage. This is definitely not the case here.

Steps to reproduce

1. Submit a front end entries/saveEntry with an assets file field.
2. Have a plugin listening on entries.onBeforeSaveEntry, and calling setContentFromPost on the $entry.

Additional info

• Craft version: 2.6.2991
• PHP version: 7.1
• Database driver & version: mysql 5.7

[brandonkelly]

@andris-sevcenko here's what's happening:

1. EntriesController::actionSaveEntry() calls the entry’s setContentFromPost() (via _populateEntryModel()
2. setContentFromPost() loops through all custom fields. When it gets to the Assets field, it detects that there is an uploaded file for the field.
3. AssetsFieldType::prepValueFromPost() is called, which converts the uploaded file to an asset, and passes its new ID to the entry’s setRawPostContent().
4. onBeforeSaveEntry event gets fired later on
5. Repeat step 2. (It still detects that there is an uploaded file for the field, because the $_FILES array hasn’t changed, even though its info is out of date thanks to the move_uploaded_file() call in AssetsFieldType::prepValueFromPost().)
6. The move_uploaded_file() call tries to move a file that doesn't exist

Not sure what the right fix is here. Maybe after calling move_uploaded_file() we should remove the record from $_FILES so the second time around, setContentFromPost() will just skip over the Assets field?

[andris-sevcenko]

@brandonkelly I guess in a way it makes sense to remove the record from $_FILES, since we're dealing with an Asset and it's relations from there on out.

On the other hand, it might be an unexpected behaviour if, for example, some plugin would want to act on the uploaded file post-upload or something like that.

What about removing it from $_FILES and firing an event as well to give plugins a chance to do something about it?

[timkelty]

On the other hand, it might be an unexpected behaviour if, for example, some plugin would want to act on the uploaded file post-upload or something like that.

@andris-sevcenko FWIW, it wasn't so much that I wanted to act on the uploaded file, but that my form just happen to contain $__FILES and I was calling setContentFromPost from entries.onBeforeSaveEntry. It didn't seem to matter what I passed to setContentFromPost, or if I was acting on the uploaded file at all.

[brandonkelly]

Actually removing the value from $_FILES wouldn’t work, since it turns out CUploadedFile caches all uploaded file data internally. Ended up solving this for the next release with a simple is_uploaded_file() check in setContentFromPost().

[timkelty]

@brandonkelly @andris-sevcenko after latest update, I'm now getting

Call to a member function getTempName() on array

from craft/app/models/BaseElementModel.php(900)

In this case $file looks like:

array(1) {
  [0]=>
  object(CUploadedFile)#971 (7) {
    ["_name":"CUploadedFile":private]=>
    string(12) "IMG_0015.JPG"
    ["_tempName":"CUploadedFile":private]=>
    string(14) "/tmp/phpjkperG"
    ["_type":"CUploadedFile":private]=>
    string(10) "image/jpeg"
    ["_size":"CUploadedFile":private]=>
    int(2116194)
    ["_error":"CUploadedFile":private]=>
    int(0)
    ["_e":"CComponent":private]=>
    NULL
    ["_m":"CComponent":private]=>
    NULL
  }
}

[brandonkelly]

Don, sorry about that. Fixed for the next release. (We’ll get that out asap.)