ENH leverage FormData to support file upload#1854
ENH leverage FormData to support file upload#1854lekoala wants to merge 3 commits intosilverstripe:2.3from
Conversation
|
The JS CI is failing as you haven't built the dist files. Can you please build that and include it in the PR? |
|
from what i see in the tests, it's not my changes that creates error but i've compiled the files on windows, it requires this to work otherwise you get Expected linebreaks to be 'LF' but found 'CRLF' linebreak-style could be nice to add that as a default for https://github.com/silverstripe/eslint-config/blob/1/.eslintrc.js to make it windows friendly |
559bb74 to
12d7335
Compare
I've created silverstripe/eslint-config#28 to track that. That's a very good idea. Sorry this has taken so long for me to look at. There was a merge conflict so I've rebased and re-compiled the assets. I'll try to review this properly shortly. |
GuySartorelli
left a comment
There was a problem hiding this comment.
This doesn't quiiite seem to work the way I'd expect. It does upload the file, but:
- If the file is for a relation (only tried with
has_oneso far), the file doesn't get stored against that relation. - If the file is an image, a thumbnail isn't included. Going to the asset admin section, there is no thumbnail for the image.
2 is probably out of scope for this, so I'd be okay with that being handled separately. But 1 definitely seems like it is part of the general problem this PR is intended to resolve.
The code I used is:
<?php
namespace {
use SilverStripe\Assets\File;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\FileField;
class Page extends SiteTree
{
private static $db = [];
private static $has_one = [
'MyFile' => File::class,
];
public function getCMSFields()
{
$fields = parent::getCMSFields();
// I also tried with the field being named `MyFile` without the "ID" suffix
$fields->addFieldToTab('Root.Main', FileField::create('MyFileID'));
return $fields;
}
}
}|
hi @GuySartorelli it does not seem related to my PR itself first, FileField don't need the ID suffix from what i can see, the upload part itself works fine if you set the relation as an image, thumbnail generation should work as far as i can tell (but it needs to be published in order to work, so maybe it's an ownership issue) so all good as far as it goes for this module i would say |


Description
Currently, ajax forms are submitted using serializeArray. This doesn't allow sending more advanced content, like a file upload
Manual testing steps
With this change, files are submitted properly. It also offers the opportunity to do more advanced things (eg: sending binary canvas data as a blob) and, all things considered, should probably work better because it uses a native js feature instead of jquery serializeArray
Issues
#1852
Pull request checklist