-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH leverage FormData to support file upload #1854
base: 2.3
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_one
so 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