Problem/Motivation
HTML5 intoduces "multiple" attribute to file upload elements, which can significantly improve file upload UX. Attribute is supported in most modern browsers and should gracefully degrade for those which don't support it yet.
There are a lot of issues on d.o. requesting multi-upload functionality and a lot of modules that try to solve it. There is, unfortunately, no standardized solution available. This issue focuses on introduction of "multiple" attribute in Drupal core.
Demo:
http://d8.janezurevc.name/
More info:
http://dev.w3.org/html5/spec/forms.html#attr-input-multiple
Code example:
<input type="file" multiple>
Proposed resolution
Support for "multiple" argument should be achieved in two steps:
- Add "#multiple" option to file and fmanaged_file FAPI elements.
- Use new feature of FAPI elements to improve UX of File and Image field widgets.
Remaining tasks
Implement "#multiple" option for FAPI elements.Use new feature of FAPI elements to offer multiple uploads in File field widget.Use new feature of FAPI elements to offer multiple uploads in Image field widget.Correct existing tests.Upgrade path for image field default_image settingUpdate docs.- function docs corrected to reflect changes, FAPI element doc should be updated on api.drupal.org once this gets committed
User interface changes
Multiple uploads are enabled by settings #multiple to true. managed_file element will then allow you to upload multiple files, remove some of them, add some more, ... Some typical situations are displayed on the image:
#multiple also works with file element, but with no UI changes.
File and Image fields use #multiple attribute to allow multiple uploads by default. There are no UI changes except the fact that file upload input element now allows user to select more than one file.
API changes
- file_save_upload() now returns array of file and gets new argument ($delta) if dev still needs just one file
- FAPI element 'managed_file' now returns an array instead of a single value
- Default image setting on Image field now stores array with single value instead of a single FID (reflects change in managed_file value representation).
Original report by philbar
HTML5 introduces a few new attributes to forms to improve file upload usability. The first is the "Multiple" attribute:
http://dev.w3.org/html5/spec/forms.html#attr-input-multiple
It is already supported by Safari 4 and Chrome 3. It will be supported in Firefox 3.6.
It's easy to use. Simply use the attribute as follows:
<input type="file" multiple>
Comment | File | Size | Author |
---|---|---|---|
#247 | 625958_247.patch | 10.83 KB | chx |
#247 | interdiff.txt | 795 bytes | chx |
#244 | 625958_241.patch | 10.82 KB | chx |
#244 | interdiff.txt | 438 bytes | chx |
#241 | 625958_241.patch | 10.82 KB | chx |
Comments
Comment #1
quicksketchThanks for informing me of this change, I wasn't aware of multi-file uploads in HTML5. Unfortunately this will certainly require extensive changes on the processing side as well, especially taking into consideration things like limiting the number of uploads to a certain number or getting the progress bar working with multiple files being uploaded at once. I doubt that the PECL uploadprogress extension properly supports multiple uploads yet.
Considering that nearly all of FileField has been moved into Drupal 7, and we're past code-freeze for new features/APIs in Drupal 7, I doubt this feature will ever be added to FileField in an official release. It might be helpful to develop this functionality as a experiment to see what is necessary for its implementation, but unfortunately we probably won't see this officially released in FileField, since this module will be discontinued in Drupal 7 anyway.
Comment #2
rickvug CreditAttribution: rickvug commentedMoving over to the Drupal 8 core queue. I found this issue when looking to see if something like http://www.uploadify.com was available for File field. Using HTML 5 seems like a much better solution (core worthy). Browser support is more solid than I would have imagined.
Comment #3
webchickThis is a pretty popular feature request. Marked the following as duplicates while cleaning up the upload module issue queue:
#101972: Select multiple files before upload
#452446: Select multiple files to upload
Comment #4
eojthebraveI would love to see this happen / Subscribe.
Comment #5
will_in_wi CreditAttribution: will_in_wi commentedsubscribe
Comment #6
Deciphered CreditAttribution: Deciphered commentedSubscribing.
I'm looking into possibilities of how to handle multiple uploads for Drag'n'Drop Uploads and this would probably be the best case scenario. If I get impatient/more time I might even have a whack at it myself, though I couldn't expect this to be a simple task.
Comment #7
GrimSage CreditAttribution: GrimSage commentedgoogle implemented this in gmail; file attachments now support multiple file uploads with upload progress. I dont know if this would be to difficult to implement as it seems that it only uploads one file at a time, so it should be able to be detected.
There is also a jquery plugin for HTML5 Upload located
http://plugins.jquery.com/project/jquery-html5-upload
There are also some more HTML5 related jquery plugins like a drag drop area for file uploads
http://plugins.jquery.com/taxonomy/term/2813
Comment #8
philbar CreditAttribution: philbar commentedIs there a reason this needs to wait until D8?
It seems like all that is needed is changing:
<input type="file">
to
<input type="file" multiple>
Comment #9
will_in_wi CreditAttribution: will_in_wi commentedYou also have to handle the files on the server side. See comment #1.
Comment #10
Deciphered CreditAttribution: Deciphered commentedYes, there is a lot more involved than the change in #8.
Comment #11
portait CreditAttribution: portait commentedCheck out this video example... http://hacks.mozilla.org/2010/02/an-html5-offline-image-editor-and-uploa...
Absolutely mindblowing. :) Or this one... http://hacks.mozilla.org/2009/12/uploading-files-with-xmlhttprequest/
Comment #12
Deciphered CreditAttribution: Deciphered commented@portait,
Just a tip, probably best not to use short URLs when you don't need to, I nearly reported you as a spammer, but I decided to use a service to determine the real destination of the short URLs in case they where legitimate, which they where.
I would suggest changing the URLs to the real URLs so no one else jumps to the same conclusion I initially did.
And for a third video, check out what you can already do with the Drag'n'Drop Uploads module: www.youtube.com/watch?v=ws6rynGxU0I
Cheers,
Deciphered.
Comment #13
rickvug CreditAttribution: rickvug commentedToday I happened across two implementations of drag and drop, in Gmail and Campfire respectively. Relevant URLs below.
Gmail:
http://ajaxian.com/archives/drag-and-drop-file-uploads-in-gmail-using-ju...(Ajaxian+Blog)&utm_content=Google+Reader
Campfire:
http://productblog.37signals.com/products/2010/04/new-in-campfire-drag-a...
I'd expect drag and drop to becoming more mainstream as time goes on. To support this would core have to set its doctype to HTML5? What are the other dependencies? (I really need to get up to speed with HTML5).
Comment #14
3dloco CreditAttribution: 3dloco commented+1
Comment #15
mgiffordtagging
Comment #16
mattyoung CreditAttribution: mattyoung commented~
Comment #17
ari-meetai CreditAttribution: ari-meetai commentedMaybe related:
- http://the-stickman.com/web-development/javascript/upload-multiple-files...
- http://www.fyneworks.com/jquery/multiple-file-upload/
- http://aquantum-demo.appspot.com/file-upload
- http://valums.com/ajax-upload/
- http://drupal.org/project/uploadify
Comment #18
droplet CreditAttribution: droplet commented+1
Comment #19
Ken Hawkins CreditAttribution: Ken Hawkins commentedSo the module Uploadify does support attaching multiple files to CCK (yes, files, not just images.)
http://drupal.org/project/uploadify
It's not extremely active and still has issues, but it works, at least for 6.x.
So if the issue here is "Support Uploading Multiple Files for HTML5 Browsers" this could be marked fixed for 6.x, but if the issue is to do it an HTML5 way, we should re-title: "Support HTML5 multiple file upload method"
Comment #20
xandeadx CreditAttribution: xandeadx commented+1 for feature request.
Look module "Multiupload Filefield Widget" http://drupal.org/project/1115362
Comment #21
z7 CreditAttribution: z7 commentedI plan to release a small module to fill this gap in Drupal 7 (already dug into the source code of file/image fields and this seems achievable, in its most primitive form, with no core-patching needed in <16h of coding/testing).
Project URL: http://drupal.org/sandbox/z7/1348240
Any comments are welcome. If there are people planning to do the same thing, please let me know.
Comment #22
kevinquillen CreditAttribution: kevinquillen commented#21 HEAD is missing, cannot checkout.
Comment #23
JacineAnyone willing to work on this? Would be nice to have it for Drupal 8.
Comment #24
z7 CreditAttribution: z7 commented#22 Use branch 7.x-1.x (instead of trunk).
Source code is also available at https://github.com/z7/Drupal-module-html5_upload
Comment #25
grendzy CreditAttribution: grendzy commentedI just found http://drupal.org/project/multiupload_imagefield_widget , which implements the "multiple" attribute. It works well and is almost invisible, so would be a good model for core I think.
I've also linked this issue from the HTML5 initiative page: http://drupal.org/node/1183606
Comment #26
JacineAwesome. Thanks @grendzy!
Comment #27
sunNext to seconding @quicksketch in #1, I additionally want to point out:
In Drupal, the file input is not multiple. The file widget is multiple.
The widget typically contains additional properties and input fields à la "title" and "alt", and possibly more (only depends on the field widget, really).
In light of that, I'm not sure I understand how the 'multiple' attribute would work in and for Drupal. It would be helpful if someone could revise, clarify, and update the issue summary accordingly; i.e., primarily focusing on the impact of this change on simple file upload form elements, file field widgets, file upload file IDs, upload progress extensions, etc. You can edit the summary directly; merely leave a comment that you did so after doing so.
Comment #28
andypostSuppose this feature should live in contrib same as about large file upload widgets
Comment #29
grendzy CreditAttribution: grendzy commentedThe multiupload_filefield_widget adds a new FAPI element (
'#type' => 'mfw_managed_file'
), and also a widget for File and Image fields. Beyond that I don't know the answers to the questions posed by quicksketch and sun.I think it is a candidate for core, because:
<input type="file" multiple>
is becoming widely adopted (e.g. GMail), so users are starting to expect forms to work that wayComment #30
JacineTotally agree with @grendzy.
Just because it doesn't work right now, doesn't mean we can't or shouldn't do this. I think it would be pretty lame of us not to do this.
Comment #31
czigor CreditAttribution: czigor commentedHi
I'm the maintainer of http://drupal.org/project/multiupload_imagefield_widget.
I agree with #29. The HTML5 initiative includes the 'multiple' attribute as well. http://drupal.org/node/1183606
To answer the other points in #27:
1. I'm not sure what a 'file upload file ID' is. The uploaded files get their own separate fids.
2. At present there's no upload progress extension support.
One more thing: there should be a fallback to non-multiple behavior for the case of non-HTML5 compliant browsers.
With this said I'm ready to transform the module into a D8 core patch if that's the way we should proceed. Is it?
Comment #32
Jacine@czigor Awesome! Patch away! The worst that can happen is that patch doesn't make it, but quite a few of us want it. :)
Comment #33
Dave ReidI would totally welcome this to become a patch against core's 'managed_file' FAPI element that would enable multiple support if '#multiple' is TRUE. Adding a Media Initiative tag!
Comment #34
aspilicious CreditAttribution: aspilicious commentedThis one is a nice one :), please patch!
Comment #35
czigor CreditAttribution: czigor commentedI will definitely do this but that might not happen happen before May. I will assign this to myself as soon as I actually start working on it.
Comment #36
yuriy.babenko CreditAttribution: yuriy.babenko commentedsubscribing
Comment #37
czigor CreditAttribution: czigor commentedComment #38
eojthebraveI took an initial stab at this the other day and here's what I've got so far. I've added the option of #multiple attribute for #type => 'file' and thus #type => 'managed_file' elements which when set to TRUE adds the HTML multiple="true" attribute to file input elements. You can then choose multiple files and have them uploaded and saved to the DB (#managed_file) ... and it works! However, getting to that point raises a bunch of questions.
For example, we need to now provide an enhanced UI for #managed_file that lets a user remove just a single file from the set that were uploaded. (The attached patch adds the button but the remove functionality isn't complete yet.) And how do you deal with editing later? Is the user required to upload all the files again if they want to make changes? Or can they add/remove individual files from a #managed_file element?
This will also have some interesting implications for the file/image fields as it means we deal with the Field API concept of multiple values which is different then the #managed_file concept.
Anyway. Here's a start. You can see it in action by enabling the file_module_test.module. I'm happy to keep working on it though could use some thoughts about how to handle the "Okay, files have been uploaded and now I'm showing you the form again scenario." I'm thinking tackle that first and then deal with the Field API scenario which is a whole other thing.
Comment #39
sunThe questions raised by @eojthebrave are exactly those I hinted at in #27 already ;)
Comment #40
grendzy CreditAttribution: grendzy commentedeojthebrave thanks for working on this! Regarding the UI questions have you studied the code in http://drupal.org/project/multiupload_imagefield_widget ?
It supplies a new widget called "multiupload". As a core patch, we might avoid some of the duplicated code inherent in having two widgets, and use the existing file / image widget. I imagine the multiple behavior would be triggered by setting the field's "Number of values" > 1.
Screen shot follows, it has the same "Alternate text", "Remove", and weight grippie elements as a regular multi-valued filefield.
Comment #41
czigor CreditAttribution: czigor commentedThe http://drupal.org/project/multiupload_imagefield_widget is just an extension to http://drupal.org/project/1115362 (Multiupload Filefield Widget). At first glance the patch in #38 seems to have the same approach. However, #38 does not deal with the widget itself. See the MFW project. Then the questions in #38 are not really a problem: the files are just different deltas of the field. So it works just like without 'muliple' but you can upload several files at a time.
I started to make a patch for d8 but I could not finish it so I did not upload it. But I will do it in the coming weeks (unless someone overtakes me).
The d7 module has some issues but they all seem to be managable to me.
Comment #42
droplet CreditAttribution: droplet commentedhttp://www.plupload.com/ ?
#1683838: Add HTML5 Drag & Drop upload to Field file
anyone update the summary, what to do in this issue ?
- Add mutiple file upload support
- (Add drop & drag upload ? )
- new UI design
- (... ?)
reroll #40 for testing.
Comment #43
danlinn CreditAttribution: danlinn commentedHello. I've merged the above mentioned module (Multiupload_filefield_widget) with the core file module. I did it on the 7.x branch, but I'm working on the 8.x version now.
Comment #45
Dave ReidComment #46
czigor CreditAttribution: czigor commentedThis might help in the sprint.
That's the present state of the D8 port of http://drupal.org/project/multiupload_filefield_widget. Uploading several files at a time works, but nothing else (eg. editing) does. I think there's not much missing to make it usable, but I could not figure it out so far.
Comment #47
slashrsm CreditAttribution: slashrsm commentedI believe this task should be split in two parts. We should first update 'file_managed' form element to support multiple uploads. I'd implement this via "#multiple" (just like patch in #38 does), which is a standard way to achieve this kind of behaviour in From API. When we have form element working we can go on and start using it in field widgets.
If we implement "#multiple" support on a right way we are not going to break any of the existing functionality in field widgets. That allows us to completely split this two tasks.
Comment #48
eojthebraveI totally agree on the implementing the '#multiple' attribute on the Form API element first though I think we should do it on #type => 'file' and not on 'managed_file' as #file is the root element that #managed_file extends in order to do it's thing.
That was the approach I took in #38, and got file uploads working via both a #type = file and a #type = managed_file, and it's pretty slick. However with that we still need to figure out how to update the managed_file widget so that it can deal with the display / removal of multiple files at the same time. Which is the trickier part here and what comments like #1, #27, and #38 are discussing.
I think there is a bit of a disconnect here between what parts are the File FAPI element and which parts are the File Field Widget (which does currently allow multiple values but not multiple uploads at the same time.)
But. We need to solve the problems with the file Form API element first and then deal with the File Field Widget (which will probably involve some updates to Field API in order to get it working)
Comment #49
slashrsm CreditAttribution: slashrsm commentedI see two solutions for update of managed_file element:
I think that the latter is better from UX perspective, but is also a bit trickier to implement. UI could look something like this:
Another option is to have just one "Remove" button and to put checkboxes in front of each uploaded file. I can code this in the next days, but we'd have to agree on behaviour before that.
Comment #50
czigor CreditAttribution: czigor commentedSolution No. 2 is preferable because it is more user-friendly and it's even easier to implement, as you can use the existing file field widget. Please have a look at http://drupal.org/project/multiupload_filefield_widget and/or my patch in #46.
Comment #51
slashrsm CreditAttribution: slashrsm commentedFirst we have to implement support in Form API element. Then we can work on file field widget. This are two different tasks of which second depends on first one.
Comment #52
czigor CreditAttribution: czigor commentedhttp://drupal.org/project/multiupload_filefield_widget contains the Form API changes too. Just have a look.
Comment #53
slashrsm CreditAttribution: slashrsm commentedAs far as I understand mfw doesen't implement anything more (in terms of Form API element) than patch in #38 and #42. Does it implement something like described in #49? Upload, update and remove of files via AJAX?
Comment #54
czigor CreditAttribution: czigor commentedIt does not need to implement it, because core already knows it. If you have an unlimited #managed_file form element (without mfw) you can upload as many files as you want (one by one), remove the ones you want to (using the Remove button supplied for each), change their order, give descriptions to them.
The multiplie html5 attribute changes this only in one aspect: You can select several files at once for uploading. Then core takes over and you can remove them one by one, sort them or give description to them.
What mfw mostly does is performing slight changes on the core functions to handle this on the database side.
To answer the question: with mfw you can upload, update and remove files with AJAX. But mfw does not implement any AJAX magic, because it's already in core.
I suggest you to try and see it in action with d7.
Comment #55
slashrsm CreditAttribution: slashrsm commentedYou are confusing Form API element and Field widget. Form element can be used on the every form in the system. Field widgets are used "only" on entity edit forms for fields attached to them. Widgets use form element to achieve it's goal.
My belief is that we have to properly implement form element first. This will allow people to use "multiple" argument on their forms and also allow us to have it on field widgets. It seems like a logic sequence to me. I also think it is not the right time to focus on field widgets, as file/image fields might change in D8.
Keeping this in mind I implemented a working prototype of "managed_file" element that supports multiple uploads. It is based on patch in #42. Multiple uploads are enabled by settings #multiple to true. Element will then allow you to upload multiple files, remove some of them, add some more, ... Some typical situations are displayed on the image:
Some things should be taken into consideration when testing this:
Comment #56
Deciphered CreditAttribution: Deciphered commentedThe most obvious thing I can see missing from your prototype images is the drag and drop ordering or any form of weighting, which is a fairly important piece of functionality, especially if people are using the first image as a primary image in a view, formatter, etc.
Would it not be better, and easier to implement, if you just have the uploading managing multiple files, but after they've been uploaded they are treated as before, as individual items, which will allow for the standard ordering/weighting, and any additional fields, such as Alt, Title and special fields for contrib modules (Insert module, Wysiwyg Fields module, etc).
Otherwise, this will potentially break a lot of contrib modules and standard use cases.
Comment #57
slashrsm CreditAttribution: slashrsm commentedDrag and drop ordering is QL. I do not see any reason why we wouldn't add that.
Displaying alt, title, weights, ... is done by field widget, not by form element (see my comments #51 and #55). File/Image widgets use this element, but since this patch do not change behaviour for single uploads it shouldn't be hard to keep current experience (see explanation below).
I left UI behaviour for single uploads (default behaviour) the same, so it should not break UI/use cases at all. It is problem, though, that element now returns array of fids instead of one fid. We could provide backward compatibility layer if we'd always check and keep returning single fid when using element for single uploads. This would add some more code and probably make it less readable, so it is a tradeoff to think about it.
Comment #58
Deciphered CreditAttribution: Deciphered commentedI guess my point is though, if you have the multi-field as the upload widget, but then once you upload multiple fiels they are instantly treated as individual files (as they currently are) you don't need to re-implement anything, nor add any backwards computability.
I also understand the need to define the difference between the Form Element and the Field Widget, but I think you should focus on the Field Widget experience first, as anything you apply to the Field Widget should translate back to the Form Element. Whereas a single Form Element for multiple fields is bound to complicate matters for the Field Widget(s).
Regardless, I do like the look of where things are going, I just want to make sure it's not going to cause nightmares if these issues aren't addressed.
Will try to take a look at the patch ASAP.
Comment #59
slashrsm CreditAttribution: slashrsm commentedAs far as I understand File/Image widget implementation it actually uses more form elements. One for upload and one for display of each uploaded file. If that's true, I do not see much problem here. We could just change upload element to be multiple and still use the same principle for displaying (single file form element for each uploaded file).
I will research this in next days and get back with more answers.
Comment #60
czigor CreditAttribution: czigor commentedSorry, I was a bit slow to get this, thanks for the clarification!
Comment #61
slashrsm CreditAttribution: slashrsm commentedFinally found some time for this. Fixed some bugs, rerolled patch against latest 8.x and fixed tests in FileManagedFileElementTest.php, which tests managed_file FAPI element. Also partly fixed some tests in SaveUploadTest.php.
Will update issue summary and continue working on this.
Comment #61.0
slashrsm CreditAttribution: slashrsm commentedCreate summary using standard template.
Comment #61.1
slashrsm CreditAttribution: slashrsm commentedCorrectly format UL.
Comment #62
eojthebraveI like the direction this is going! Awesome work.
Also, I agree that we should focus on making the Form Element work independent of the File Widget and then make use of the new form element within the file widget (which also does a lot of other possible things, i.e. alt and title fields for image uploads.)
If the form element had '#display_files' => FALSE or something similar it would then be possible to have the File field widget still use the #managed_file element but handle the display/sorting and other things it does just like it does currently. All you're really replacing is the upload button which would allow you to upload multiple files at the same time and would be no different than someone uploading multiple files individually.
Comment #63
slashrsm CreditAttribution: slashrsm commented@eojthebrave: Sure..... Filed widget experience should stay exactly the same (except multiple uploads support).
I was thinking about file_save_upload(). I think it should return array of files now, as we changed how FAPI element works. In attached patch I added this, but added another argument to it, which will allow devs to request only one specific file from that array (usually the first one when #multiple => FALSE).
Comment #63.0
slashrsm CreditAttribution: slashrsm commentedAdded UI changes part.
Comment #64
slashrsm CreditAttribution: slashrsm commentedI started to work on multiple uploads on FIle field. Feature is mostly implemented in attached patch. If you apply patch to latest 8.x, create multivalue File fileld and try to upload multiple files via standard upload element you'll see what I mean.
There is still a lot of work to do (Image field, validations, tests, ...), but anyone can get the basic idea from this patch.
Comment #65
slashrsm CreditAttribution: slashrsm commentedAdded some extra validation when field cardinality is between 1 and unlimited. Also corrected comments in file_save_upload(), which were a bit outdated now.
File field should be working now. Moving on to Image field.
Comment #65.0
slashrsm CreditAttribution: slashrsm commentedUpdated issue summary.
Comment #66
aspilicious CreditAttribution: aspilicious commentedLets test this
Comment #67
slashrsm CreditAttribution: slashrsm commentedAs issue desc says tests still need to be fixed, so this will definitely fail...
Comment #68
aspilicious CreditAttribution: aspilicious commentedI applied this patch to my d8 sandbox.
Also when uploading an image to the article content type
Last but not least. Where can I choose the multiple widget in the UI?
Comment #69
slashrsm CreditAttribution: slashrsm commentedImage field is not implemented yet (working on that). File field should work.
Comment #71
eojthebraveI wonder if it makes sense to try and split this up into two patches. One that implements the Form API element and all the related logic/tests/etc, and then another that adds support for the file and image Field API widgets? It might make it easier to review at least.
Here's a couple of things I noticed after a quick glance. Mostly cosmetic. Keep up the good work!
There's an extra space in this sentence.
This comment is a little confusing. How about something like "Delta of the file to save or NULL to save all files. Defaults to NULL."
I also think that $delta may be a better variable name in this instance. It's a bit more consistent with other similar code and is a common pattern that people will have seen before and thus better understand. However, I also see that $delta is being use elsewhere in this function so it could be confusing. Just thinking out loud now. :)
I think NULL may be a more meaningful value here than FALSE personally.
has should be have
missing . at end of line
You can probably use drupal_map_assoc() here.
The wrapping could be fixed on this comment.
Powered by Dreditor.
Comment #72
slashrsm CreditAttribution: slashrsm commentedThank you for your reviews!
I was also thinking about this. I was also thinking about splitting this into two issues, but then I realized that both parts are so related to each other that it does not make sense. If I implement FAPI element changes I must also change logic in widgets as those need to look for input values differently (currently we have single value, now we return array). If I do not do that all tests will fail, so I believe it does not make sense.
However, I agree that review would be much easier. I believe that all changes for FAPI elements happen in includes/form.inc, includes/file.inc and modules/file/file.module. All other changes are either test fixes or field widget changes. Does this information help?
Image field widget should now work. I also fixed all comments you posted in the previous comment and some tests. Let's see what test-bot has to say about that...
I think that drupal_map_assoc() cannot be used here as it will use values for keys. Values are objects in our case, which results in a fail. IHowever, I refactored this part a bit and it should be much nicer now. Tell me what you think...
Comment #72.0
slashrsm CreditAttribution: slashrsm commentedUpdate progress
Comment #74
slashrsm CreditAttribution: slashrsm commentedLet's see if we fixed those nasty tests....
Comment #75
slashrsm CreditAttribution: slashrsm commentedFixed minor bug that I produced in the previous patch. Also fixed a possible notice.
Tested uploadprogress and it looks to work fine with APC and PECL uploadprogress.
Comment #75.0
slashrsm CreditAttribution: slashrsm commentedUpdate changes.
Comment #75.1
slashrsm CreditAttribution: slashrsm commentedUpdated issue summary.
Comment #76
slashrsm CreditAttribution: slashrsm commentedDemo page: http://d8.janezurevc.name/
Comment #77
rooby CreditAttribution: rooby commentedDemo works well for my brief test.
The only concern I would have initially is that it really needs upload progress to be user friendly.
Comment #78
mikeytown2 CreditAttribution: mikeytown2 commenteduploading multiple gif's to the field (only accepts txt, jpg, png) caused this in my browser to pop-up. The error message is there, just isn't handled correctly.
I formatted the json below:
Comment #79
mikeytown2 CreditAttribution: mikeytown2 commentedUploading 2 blank txt files caused this output
Comment #80
mikeytown2 CreditAttribution: mikeytown2 commentedUploading 1 jpg gave this output
Comment #81
slashrsm CreditAttribution: slashrsm commented#78: I get the normal error message if I upload gif (see screenshoot).
#79, #80: Am unable to reproduce. Empty txt files and jpg uploads fine.
Are you sure JS loaded correctly?
Comment #82
mikeytown2 CreditAttribution: mikeytown2 commentedOpening up the net panel (in firefox) and I get 200's on all items.
Windows 7 32bit.
Works in Chrome 24.0.1312.57 & Safari 5.1.7
Broken in Firefox 18.0.1, Opera 12.13, & IE 9 (single file upload).
Comment #83
slashrsm CreditAttribution: slashrsm commentedInspecting this...
Comment #84
slashrsm CreditAttribution: slashrsm commentedHm.... I was trying to reproduce the same errors you described, but I was mostly unsuccessful. I did, however, notice some weird behaviour in IE9, which is there also on 8.x. I suspect that could be some other issue, not related to this patch. Can you check please?
Comment #85
mikeytown2 CreditAttribution: mikeytown2 commentedWith Firefox the issue has to do with this addon
https://addons.mozilla.org/en-us/firefox/addon/jsonview/
Here is a video of what happens in opera
http://www.youtube.com/watch?v=lmBF_8I9PbM
Video in IE
http://www.youtube.com/watch?v=slWldwQ6Bds
My guess is the headers being sent is causing an issue.
http://www.devcha.com/2012/02/internet-explorer-offersprompts-json.html
Comment #86
slashrsm CreditAttribution: slashrsm commentedI am getting the same error. The thing is I also get it if I test on un-patched 8.x branch, which makes me think that it is a separate issue. Can you confirm this?
If I press "Save and publish" after that node gets saved correctly, so I suspect AJAX request finishes fine, but is not interpreted correctly.
Comment #87
mikeytown2 CreditAttribution: mikeytown2 commentedThe backend works correctly. It is a separate issue; one with core. My guess is something like this
JsonResponse::update
Comment #88
slashrsm CreditAttribution: slashrsm commentedWe should probably report this as a separate issue then.
I added default_image setting upgrade patch for image field.
Comment #89
mikeytown2 CreditAttribution: mikeytown2 commented#1912226: Internet Explorer offers/prompts JSON output for download
Comment #91
slashrsm CreditAttribution: slashrsm commentedLet's see what test bot says about this...
Comment #91.0
slashrsm CreditAttribution: slashrsm commentedAdd demo
Comment #91.1
slashrsm CreditAttribution: slashrsm commentedUpdated issue summary.
Comment #92
tsvenson CreditAttribution: tsvenson commented@slashrsm asked me to do a quick UX review of this patch on the content adding form using a demo site he set up. I didn't have have access to admin, nor have seen the config options available.
Adding multiple files was easy (Chrome on Windows 7). Only found a few small issues.
A great UX improvement to the fieldgroup would be if it also could give a hint about the content of it, such as:
This way I will know if there are any files or not in there without having to expand it.
Comment #93
slashrsm CreditAttribution: slashrsm commentedThanks @tsvenson. I actually noticed that "collapsed field widget" problem also. I opened another issue for it: #1912784: Add counter to fieldsets of files and images.
Auto-upload is also an interesting feature, but there are probably also a lot of arguments against it also. However, I think that this feature exceeds the scope of this issue and deserves it's own.
Comment #94
tsvenson CreditAttribution: tsvenson commented@slashrsm, just want to say thanks back for making it so quick and easy for me to have a look at this. The fact that you set up a demo site saved me probably 30-45 minutes to just get a test environment up and configured before being able to look at it.
So, my personal UX of doing the UX review was excellent :)
Comment #95
droplet CreditAttribution: droplet commentedI created an issue about Drag & Drop upload but closed and point to here.
#1683838: Add HTML5 Drag & Drop upload to Field file
Comment #96
chaloum CreditAttribution: chaloum commentedJust did a manual review of the patch but got this error
How to reproduce the error
Using the patch from #91
I using a Mac OSX lion Safari. I havent tried the other browsers yet.
Comment #97
slashrsm CreditAttribution: slashrsm commentedHm... patch in #91 had some problems applying to the most recent 8.x. Reroll is attached.
I was trying to reproduce your error with most recent 8.x and most recent patch, but I was unsuccessful. Can you try again with the latest patch and latest 8.x code? Could you please enable Devel's backtrace option for errors to see which function calls check_plain() with wrong arguments?
Comment #98
slashrsm CreditAttribution: slashrsm commentedThis should be the right patch.....
Comment #99
micheas CreditAttribution: micheas commentedHaving used the file api and pluploader for a couple of sites I have a few d7 issues that I have run into that I would at least like the developers to be aware that the code will probably be used/abused in the following ways:
Uploading large files directly to amazon s3. (3GB video files) The issues with this is that there is no easy way in d7 to have drupal call aws at the end of the upload. Or more generally I guess there is no upload start, upload end hooks in the d7 api.
On one site I had pluploader set so that a user could drag a large number of files into the uploader (say 500, but there was no practical limit that I hit while testing.) The files were uploaded and processed serially so that the total size did not exceed the php memory limit. For each image uploaded a node was created so that various cck fields could be related to the images (date, time related nodes, related taxonomy terms, copyright, etc.)
I would strongly suggest that the test to see if this works includes uploading files that individually fit in the php memory limit but collectively exceed it. If the limit is the total size of the files uploaded, people processing large files will have to disable this functionality and it could become a common issue from end users.
Thanks for taking the time to read my comments and even more thanks for working on this.
Comment #101
chaloum CreditAttribution: chaloum commentedjust manually tested patch #97 on the the current D8 rebase and the multi file field behaved correctly BUT I could only choose one file.
Comment #102
slashrsm CreditAttribution: slashrsm commented#97 was an empty patch. When I realized my mistake I uploaded #98 which is fine, but failed some upgrade path tests.
#102 is based on #98. Let's see if it fixes test fails. #102 is the also the right patch to test. Thank you for your help!
Comment #103
chaloum CreditAttribution: chaloum commentedOn a rebased version of D8 I installed patch 102 and successfully uploaded multiple files.
I also successfully tested a group of files where the files (between 600KB and 900KB) are less than the file upload limit but were the group of files exceeds the the max file size (2MB).
Comment #104
chaloum CreditAttribution: chaloum commentedalso if I can add a suggestion the field group defaults to closed/collapsed and when uploading you need to open it and upload the files. After the files upload the page refreshes and the field groups defaults to collapsed. is there a method to maintain the state for the field groups so the end user doesn't need to keep opening the group. It would be better if it remembered if the user had closed or opened the group and kept that state when the page refreshed.
Comment #105
slashrsm CreditAttribution: slashrsm commented@tsvenson already suggested that in #92 and we created new issue for that in #93. Your suggestions there are very welocome!
Comment #106
tsphethean CreditAttribution: tsphethean commented@slashrsm - Sorry for delay, I finally got round to reviewing this! :)
One thing I noticed, is that if number of selected files exceeds limit on the field, currently we accept the first x number of files until the limit is reached - and we just output the count of the number of files that were successfully uploaded. Do you think we should display more information than this (i.e. the files names of the files which weren't uploaded?) or maybe even hard stop completely if the files a user has selected will exceed the limit and prompt them to remove files until under the limit?
I guess this is mitigated by showing the uploaded files in the widget, but might be helpful for a user to know which files weren't uploaded, particularly for high numbers of files?
Alternatively maybe the file/image field widget should display the number of allowed files if multiple are allowed, in the same space as the field help text is displayed?
I like the fact then once the full number of images/files is reached the file chooser disappears, although this threw me initially because the limit wasn't displayed anywhere, maybe another reason for displaying number of allowed files?
Possibly out of the scope of this issue, but worth considering when we're looking at make inputs of type file HTML5 valid, we should populate the "accepts" attribute with the mime type of files which are allowed - that might get complex as we'd need to move from an extension based validation to mime type based, but should be possible? New issue?
From a code perspective, I'm not an expert on this area of Drupal, but the code looks solid and nothing leaps out as wrong. Might be worth someone with a bit more experience in fields and file management to cast an eye over it though. The patch applies cleanly and all the manual tests I've run work without error.
Off topic - I'd not used it before today but http://simplytest.me/ is simply awesome for testing this kind of thing!
Comment #107
slashrsm CreditAttribution: slashrsm commentedThank you for your review. I agree that we should tell some more info when user uploads too many files. Attached patch displays list of removed files along with the message. I also added info about maximum number of files that can be stored in a field.
I think that there is some attribute for that limits number of files that can be uploaded. Downside is that it is only supported in Opera. We could add some extra JS validation for that. This will be much better in terms of UX, since it would inform user about exceeded file count limit before the actual upload. I'd say that we can do this in a separate issue if this gets committed, since I'd not make this patch even bigger.
I opened new issue for "accepts" argument: #1917270: Add support for "accepts" attribute for file upload FAPI elements
Comment #108
tsphethean CreditAttribution: tsphethean commentedThat looks good to me - the message is much more helpful and the limits displayed is good UX.
Doing the validation before the upload button is submitted would an awesome next step, but I agree that should probably come in a new issue.
Thanks for opening #1917270: Add support for "accepts" attribute for file upload FAPI elements.
Great job.
Comment #109
sunWe have to use the update helper functions here.
Since the field processes and massages the values already, I do not really understand why all widgets and everything is touched in any way to use fids instead of fid.
The field item that a widget operates on is still a single file.
Comment #110
slashrsm CreditAttribution: slashrsm commentedThank you for your review!
Could you point me somewhere where I can find more info about that?
When I enabled multiple uploads for FAPI elements they needed to return arrays instead of a single value. It felt natural to me to rename 'fid' to 'fids' to indicate that.
FAPI elements are used in field widgets, so I needed make a change there also. Naming was unfied before that change. FIelds storage expected ID to be passed as 'fid' and FAPI element were passing the value using the same key. When I changed behaviour of FAPI elements I had two options:
- change field storage to expect 'fids' as key or
- convert 'fids' that came from FAPI elements to 'fid' that is understood by storage system.
I decided for second option for various reasons, includding:
- DB stores one value per row. Changing 'fid' to 'fids' would rename column name, which doesen't seem logical.
- Changing 'fid' to 'fids' at storage level would change structure of file object to something like this:
which also does not seem logical as every items stores just one file.
Comment #111
sun1) @see http://api.drupal.org/api/drupal/8/search/_update_7000 (or rather field.install)
2) The storage should definitely contain a 'fid'. It is possible that the Form API widget internally uses #files and thus 'fids', but the moment the submitted form values are processed, massaged, and extracted, these values should be translated into individual file items already. Thus, when we reach the Field API layer and the code paths of the field widget/formatter/type, the translation happened already, and those should only have to care for single/individual files. (That is, unless I'm missing something big.)
Comment #112
slashrsm CreditAttribution: slashrsm commentedWill change update function and post it a bit later.
It is exactly what I do. When we're in from part of the process I use 'fids' everywhere. This ensures that values are preserved correctly through multiple requests (upload, remove, etc.). When form gets submitted I translate "fids" to "fid" in massageValues(), which basically prepares values for storage system.
One thing to be noticed here is the fact, that widget basically uses N+1 managed_file elements (N being number of uploaded files). Even items that just display files that are currently stored in the field are technically managed_file FAPI elements with different theme function applied to it. That is the main reason why I needed to use 'fids' also for those even if they store only one value each.
Comment #113
sunWell, as I've outlined in #109, various Field API callbacks/methods are changed in this patch to use 'fids' instead of (a single) 'fid'. Everything in the Field API code space should operate on a 'fid' as previously. In fact, most of the changes in that space almost look like false-positive changes stemming from a global search+replace...?
Comment #114
slashrsm CreditAttribution: slashrsm commentedAll callbacks that were changed operate on FAPI forms/elements or on values extracted from them. The problem is that this FAPI system often relies on this values during multi-step form operations. If I'd change this too soon I'd break passing of values through multiple requests. That's why I perserve 'fids' until form is submitted at the end.
Changes were not done via search/replace technique. It was try to upload, check errors that were returned and go fix them in code approach. I tried several different approaches for different parts of this patch and this one looked and works the best.
Comment #115
slashrsm CreditAttribution: slashrsm commentedHere is the patch that uses helper function to load fields in update hook. I was actually able to replace just one call. Do you see any other solution?
Comment #116
slashrsm CreditAttribution: slashrsm commentedDo you need any more info?
Comment #117
eojthebraveThis is looking really good. Couple of nit-picky comments on some of the comments and strings below. Keep up the awesome work slashrsm!
Just nit-picking here but this could be shorter.
return isset($delta) ? $upload_cache[$source][$delta] : $upload_cache[$source];
This comment needs to wrap at 80.
I find this help text to be a little bit hard to follow. What if it said something like "Field %field can only hold @max values but there were @count uploaded. The following files have been omitted as a result: %list."
This comment needs to be wrapped, and I think is missing a "the". ... uploaded via the same widget ...
This should probably read, "This is used multiple times so only implode it once.", The comment that's there currently is worded strangely at least.
Looks like this can probably get removed. However if it does need to get left in the comment should be wrapped at 80 characters.
"more than one file at a time", and "returns an array of fids."
Powered by Dreditor.
Comment #118
Deciphered CreditAttribution: Deciphered commentedWhile there doesn't appear to be anything about shorthand if/else statements in the Drupal coding standards, my experience has been they aren't the preferred approach due to readability.
However, at the minimum it could be changed to:
No need to specify the else statement here as it's a return. Reduces the code by two lines and it's still nice and readable.
I agree with all the other points however.
Comment #119
slashrsm CreditAttribution: slashrsm commentedComments included in patch.
Comment #120
slashrsm CreditAttribution: slashrsm commentedThis was already RTBC and was changed to needs review for no special reason.
Comment #121
Dave ReidThis wasn't RTBC on Feb 18 though.
Comment #122
renat CreditAttribution: renat commentedWell, actually it was set to RTBC at #108.
Comment #123
slashrsm CreditAttribution: slashrsm commentedReroll against latest 8.x.
Comment #124
slashrsm CreditAttribution: slashrsm commentedAnother re-roll...
Comment #125
mgiffordThat seemed to work for me in simplytest.me. What else needs to happen to get this RTBC?
Comment #126
slashrsm CreditAttribution: slashrsm commentedThank you for your help! Someone needs to change status to RTBC :).
I believe that we still need some proper code review. Some people have looked at the code, but I assume that no one did it thoroughly.
Comment #127
quicksketchIt looks like this code rearrangement undoes the primary purpose of the code. I don't think $ajax_settings is going to affect the remove button if it's only updated after the remove button has been set.
"Comvert" => "Convert". Missing leading space on subsequent lines docblock lines.
I'm personally not a fan of this kind of anonymous function in PHP. Besides avoiding unnecessary syntax, it's more succinct as a normal loop:
I'm not sure that forcibly adding
[]
to all file fields is a good move. If you were working in situations where you needed to build a very-specific #name attribute (say for an existing system or 3rd-party API), forcibly altering the #name attribute seems like a bad idea. I'd like to see the extra [] only added if #multiple is TRUE and adjust the code in file_save_upload() to work with single-file uploads and multi-file uploads. It's a good thing if we store the list of files as an array internally (especially for file_managed elements), but forcing an array structure on the front-end seems aggressive.Missing a space after
if
.Overall this looks great! I love the improved workflow for uploading multiple files. I'll need to test this locally to see how managed_file elements look with #multiple enabled when in non-field situations. It sounds like the progress bar works with multiple files too, though unfortunately it looks like the option to use a progress bar isn't available on simplytest.me, again it'll need to be tested locally.
Comment #128
slashrsm CreditAttribution: slashrsm commentedThank you for your review. I really appreciate it. I fixed all things you mentioned in your review.
Change of name attribute may have broken some tests (I fixed them but I am not sure if I covered all changes). Let's see what test bot has to say about that...
Comment #130
slashrsm CreditAttribution: slashrsm commentedThis should fix tests.
Comment #131
slashrsm CreditAttribution: slashrsm commentedReroll....
Comment #132
Wim LeersIt looks like this has had many reviews already, plus this is a MASSIVE patch, so I tried to review 2 aspects: 1) test coverage, 2) details.
Upon reading the issue summary, I thought this was going to be a small patch — how very wrong I was! :D Very nice work here!
1) It looks like existing test coverage has simply been updated, and no new test has been added to test #multiple file uploads?
2) See below:
Does this really need to be hardcoded? Shouldn't this at least be retrieved from the config system?
Why are we explicitly checking certain scripting language extensions here? Why are they hardcoded?
It feels strange that we're dynamically overriding settings here.
(nitpick) s/URI/file URI/
s/==/===/, because that's what the if-test tests.
s/==/===/ ?
s/==/===/ ?
"user has" should be on the first line.
Again, wrong wrapping.
s/==/===/
?
s/some times/sometimes/
?
s/!=/!==/
?
s/==/===/
?
Here it's "fids", earlier it was "FIDs".
Closure! :)
I think there need to be spaces around the return statement.
Needs to be rephrased.
Wrapping needs to be fixed.
Comment #133
tkoleary CreditAttribution: tkoleary commentedLinking to Usability group seven style guide http://groups.drupal.org/node/283223. There are already some designs around this interaction.
Comment #134
tkoleary CreditAttribution: tkoleary commentedImage from that discussion:
Comment #135
slashrsm CreditAttribution: slashrsm commentedThank you for this review Wim. I was also surprised when I saw how big it became. It looks so simple things but when you start to go deeper and deeper you realize it influences a lot of things. I believe it will improve file upload UX significantly, so I'm really looking forward to get this in.
Tests are only corrected to work with new FAPI element. I tried to add tests for multiple uploads but it looks like Simplenews do not know how to work with multiple files in one element. First I wanted to fix this, but then I stopped. Didn't want to make this patch even bigger. I will be happy to work on this in a separate issue when this gets in. Sounds logical?
Security features in file_save_upload() have not changed. They are staying exactly like they appear in D7 and D8.
Allowed extensions are there for security reasons if developer forgot to limit them in his code. That prevents people from uploading anything they want. The actual configuration should be in code that implements upload (as most if not all modules that deal with files do - core's file.module and contrib's file_entity.module for example).
This could make sense to be configurable, but again, it is here already and I think it is out of the scope of this issue.
About all == vs. === comments. Most of them are already here. I added two (one is changed in attached patch). Do you really think that we need to force type match on all comparisons? This for example
'1' == FILE_EXISTS_REPLACE
is TRUE while this'1' === FILE_EXISTS_REPLACE
is not. I agree it is good practice to use defined constants, but I see no benefit using type matching comparison operators all over the place. I might be wrong, though...Attached patch fixes all other comments.
@tkoleary: This looks really, really nice!
Comment #136
slashrsm CreditAttribution: slashrsm commentedComment #137
quicksketchI agree with @slashrsm on most all counts. I think using === in most places is a good thing, but it gets pretty messy in PHP when dealing with integers vs. their string equivalents. Currently in FAPI, we don't enforce any kind of strict checking. You can say #required = 1/TRUE/'1' and they all work. For consistency, strict boolean checking shouldn't be required for #multiple or other FAPI properties unless they're all made that way.
@tkoleary cross-reference has some additional improvements, but they should certainly be separate from this patch. Styling file form elements has some pretty serious challenges because you're not allowed to set an
input="file"
element via JavaScript. Additionally browsers don't let you style file elements or even change what the word "Browse" says on file browse buttons, so there are a lot of challenges there that should be separate from this issue.I'm trying out the latest version of the patch to test the managed_file element when used stand-alone (as shown in #55). It's looking really good. Validation properly fails on only one of 3 files when one file is the wrong extension. The non-JS version works even with multiple files. Unfortunately I found that removing an individual file removes all the files currently attached, no matter which checkboxes you select. Additionally, test coverage is incomplete, since this particular situation should probably have been tested already. I'm working on a reroll.
Comment #138
quicksketchThis patch introduces a strange use of $form_state['storage']:
This
$form_state['storage']['managed_file_values']
variable is never actually referenced, except for in the situation where files are being removed, but it doesn't work in that one situation. Seems like we should remove this introduction of $form_state['storage'] and go back to using values passed back and forth in $form_state['values']. Those values are already always set, so as far as I can tell, the use of $form_state['storage'] is entirely unneeded.Comment #139
quicksketchThis patch removes the new use of $form_state['storage'] I mentioned in #138. It also makes the case of removing a single file to a multiple file element (just an element, not used as a Field-field) only delete that one file instead of all of them. Added test coverage for the #multiple property, adding an extra loop in all our existing coverage so every combination of #extended, #tree, and #multiple is tested in AJAX and non-AJAX situations. Because #multiple introduces some new situations (like removing a single file), I also added a set of new assertions for checking individual file uploading and removal.
Comment #140
quicksketchRe @slashrsm in #135:
Though my my latest patch expands test coverage, it doesn't attempt to upload multiple files at the same time into a #multiple file element. It uploads one file at a time to the same field. So it checks that multiple files are supported, but not all in a single upload at the same time. Expanding Simpletest to support multiple files at the same time really could spin into a whole new set of problems, so I think separating that issue makes a lot of sense.
Comment #141
jibranWow just played with it on simplytest.me amazing work. All the files are uploaded in one go :).
Dono related to this issue but getting this error while viewing the node
Warning: imagecolorsforindex() [function.imagecolorsforindex]: Color index 127 out of range in Drupal\system\Plugin\system\imagetoolkit\GDToolkit->createTmp() (line 257 of /home/s7c40bb096d80507/www/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.php).
Comment #143
quicksketchFixing notices. @jibran: I don't think that's caused by this patch, but perhaps it will be corrected by this version. The previous patch had an issue when dealing with single-file file fields.
This patch also almost introduced additional misuse of #array_parents when #parents should be used. I've fixed all places where that was mixed up, including the previous mixups, so this patch encompasses #1468686: file_field_widget_submit(), file.field.inc using the wrong parent key now too.
In short:
- Use #array_parents when trying to get an element out of $form because #array_parents matches the form structure.
- Use #parents when trying to get a value out of $form_state['values'] because #parents is used as the "name" attribute, thus $form_state['values'].
Comment #144
slashrsm CreditAttribution: slashrsm commentedThanks for great work. Remove operation on multiple upload element now works flawlessly.
I take entire responsibility for mess with #array_parents and $form_state['storage']. I am, due to the size of this patch, a bit surprised that I only messed up with those two things. Thank you for pointing this out!
In case anyone interested in difference between #parents and #array_parents:
- #279246-3: FAPI #array_parents is not documented
- http://drupal.org/node/48643
Comment #145
Wim Leers#135: Thanks — I agree with your rationale for not changing certain things I had asked to be changed.
Thanks, quicksketch, for your far more useful review (and rerolls) on this issue!
What's left here to get this to RTBC? :)
Comment #146
slashrsm CreditAttribution: slashrsm commentedI very glad to see interest from you in this. It looks very well and I'm thrilled about that.
You're the boss. :)
Comment #147
tkoleary CreditAttribution: tkoleary commented@slashrsm
Credit goes to Yoroy, Bojhan and ry5n on that design.
Comment #148
tkoleary CreditAttribution: tkoleary commented@quicksketch
Yes, that makes sense. Just wanted to make sure that everyone on this issue had visibility so that we don't introduce a task flow that would be out of sync with the proposed UI.
Comment #149
quicksketch@tkoleary: Yep understood. This patch actually moves us a lot closer to the proposed design because it eliminates the "Add another item" button for files, since you only need one field now to upload however many files you want. Overall it's a great step in UX.
Even though I did the last reroll, I think this is really @slashrsm's patch and I don't think it's out of line for me to move it to RTBC.
Comment #150
webchick#143: multiple_fapi_uploads_625958_143.patch queued for re-testing.
Comment #152
andypostpatch needs re-roll
Comment #153
slashrsm CreditAttribution: slashrsm commentedReroll.
Comment #155
slashrsm CreditAttribution: slashrsm commentedMy bad. Should be fine now.
Comment #157
slashrsm CreditAttribution: slashrsm commentedField API CMI patch made _update_7000_field_read_fields() obsolete so using field_read_fields() instead.
Should pass now.
Comment #159
moshe weitzman CreditAttribution: moshe weitzman commentedJust one fail.
Comment #160
slashrsm CreditAttribution: slashrsm commented#157: multiple_fapi_uploads_625958_157.patch queued for re-testing.
Comment #161
slashrsm CreditAttribution: slashrsm commentedPasses without any problems locally. Trying to retest.
Comment #162
eojthebraveBack to RTBC. Looks like this was just a simple "chasing HEAD" fix and the most recent patch doesn't introduce anything new and passes tests.
Also. OMG slashrsm++, way to step up, own this patch, and carry it through. Your rock!
Comment #163
webchickAwesome, thanks! Restoring RTBC, per #162. :)
Can't do much about this right now because of thresholds, but this is at the tippy-top of my list once features are back on the table again!
Comment #164
slashrsm CreditAttribution: slashrsm commentedThank you @eojthebrave and @webchick. Very happy and hoping to see this in soon.
Comment #165
moshe weitzman CreditAttribution: moshe weitzman commentedWe already support uploading multiple files in same form. IMO, this can qualify as an HTML5 port and thus isn't subject to thresholds.
Comment #166
webchickYou know what? I can get behind that.
Committed and pushed to 8.x. Thanks!
GREAT work, all! :D
Comment #167
quicksketchLOL. Okay well I won't complain. Ginormous thanks to @slashrsm! Incredible work!
Comment #168
droplet CreditAttribution: droplet commentedWhere's the drag & drop, anyone point me a new issue ? or should I reopen my one ? #1683838: Add HTML5 Drag & Drop upload to Field file
Thanks!
Comment #169
Wim LeersAwesome work, slashrsm! :) Thank you!
Comment #170
quicksketchI reopened #1683838: Add HTML5 Drag & Drop upload to Field file to address drag and drop style uploads (which is in our D8 style guide for Seven also).
Comment #171
slashrsm CreditAttribution: slashrsm commentedWow! You made my day :). Thank you all!
Now we need to document #multiple on api.drupal.org. I'd swear I once found one project that is used for that, but now I can't remember which was. Can anyone give me a hint?
Comment #172
yched CreditAttribution: yched commentedA couple issues with the newly added file_update_8002():
- There seems to be no reason for "if (module_exists('field_sql_storage')) {".
This is typically done for updates that need to work on stored field values, and usually only do so on fields that are stored in sql.
But it's not the case here, the function is only changing field and instance definitions,
- field_read_fields(), field_update_field(), field_read_instances(), field_update_instance(), are API functions that are not supposed to be used in updates. They are also on their way out after #1735118: Convert Field API to CMI.
Attached patch moves the code to direct CMI manipulation, and adds an explicit update dependency on the "field to CMI" update (the update currently implicitly expects to run after, since the field_CRUD_*() functions now operate on top of CMI).
#1969662: Provide new _update_8000_*() CRUD helpers for Field and Instance definitions should provide helper _update_8000_field_[CRUD]() functions at some point, but not there yet.
Comment #174
andypostuse Config as object
Comment #175
yched CreditAttribution: yched commentedDoh, right. Thanks @andypost :-)
Comment #176
slashrsm CreditAttribution: slashrsm commentedTested it and correctly updates default images on field/instance.
Now I'm thinking.... should we move this to image.module since it upgrades image fields only?
Comment #177
andypostMakes sense!
Comment #179
slashrsm CreditAttribution: slashrsm commentedLet's try again.
Comment #180
slashrsm CreditAttribution: slashrsm commentedComment #181
slashrsm CreditAttribution: slashrsm commentedFound another nasty one.... Remove button stops working if we have single file_managed element with file assigned to it that is not temporary.
This seems to fix it.
Comment #182
andypostdo we have a test about file_managed element? as we need for #table #1975152: Expand render test coverage for #type table
Comment #183
slashrsm CreditAttribution: slashrsm commentedWe have it, but it looks like it's always dealing with temporary files (which is the default if you have a simple forma and you upload a file to it). Maybe we should expand that.
I found this bug when playing with image field default image configuration. Maybe we could test that a bit better and also cover managed_file and permanent files.
Comment #184
yched CreditAttribution: yched commentedimage_update_8003() needs to be renamed image_update_8001() :-)
[edit: and image_update_dependencies() modified accordingly]
Comment #186
andypostChanging functions order does not help, still
8003 - Moves image module settings from variable to config. @ingroup config_upgrade
is pendingIt seems that something wrong with update process
Comment #187
andypostOnce update number goes less 8002 - patch show this hack - tests are passes
Comment #188
yched CreditAttribution: yched commentedDoh, sorry, Please scratch #184.
I was mistakenly reading the interdiff from #177, I thought the patch was going from image_update_8000() straight to image_update_8003().
I was wrong, and didn't mean to imply that switching image updates order would help with the fails.
It might be that moving the update (and the associated update dependency info) in image.module made the dependency tree unsolvable ?
Comment #189
yched CreditAttribution: yched commented#1953404: Add config schema to field and instance config entities needed to add a 'field_type' property in the field instance config file, so the update can be simplified a bit.
Comment #190
andypostThis hack allows pass tests but needs work to solve dependency graph anyway
Comment #191
swentel CreditAttribution: swentel commentedAlso, we don't drop tables during upgrade, see #1860986: Drop left-over tables from 8.x also, but maybe that's a dedicated issue.
Comment #192
slashrsm CreditAttribution: slashrsm commentedHm... only adding image field update hook as 8003 works fine for me. Tried to upgrade form 7.x to 8.x+attached patch and from one commit before #multiple was added in #166 and both upgrades went fine.
Comment #194
slashrsm CreditAttribution: slashrsm commentedI was manually playing with upgrade test dump a bit. From what I've seen is not dependency tree that makes problems. It looks like image.module is disabled at the beginning of upgrade process (all failed tests are based on bare.minimal dump), and that's why image update hooks are not run during first run.
However... it seems like image module gets enabled during update process, which causes our update hook to appear in pending updates list after first run has finished it's job.
Comment #195
yched CreditAttribution: yched commented[edit: crosspost, #194 is spot on]
Comment #196
yched CreditAttribution: yched commentedHeh, right. user_update_8011() enables image module, setting it's schema version to 8002, which was the latest image update so far.
Then the newly added 8003 is dangling.
I think user_update_8011() should be adjusted to:
- directly use the new format for the 'default_image' settings in the $field and $instance it creates
- enable image.module with schema version 8003 rather than 8002, so that image_update_8003() is not pending at the end of the process
I'm not even sure why providing an explicit schema version is needed, since it's doomed to break again next time image.module adds an update. Just installing image.module "as usual" (i.e. with schema version = the last update func present in the module) should be enough ?
Comment #197
yched CreditAttribution: yched commentedLeft a note about that in #1894328: user -> image upgrade is broken, that introduced this hardcoded "set image schema version to 8002" in user_update_8011().
Meanwhile, bumping to 8003 here should be a safe workaround.
Comment #198
slashrsm CreditAttribution: slashrsm commented@yched: thanks for clearing this out!
Let's try....
Comment #199
yched CreditAttribution: yched commentedThe field and instance created in user_update_8011() should be adjusted to set their 'default_image' setting in the new format, since image_update_8003 will not run on them.
Comment #200
slashrsm CreditAttribution: slashrsm commentedAh... yes. Here it is....
Comment #202
slashrsm CreditAttribution: slashrsm commentedTry again....
Comment #203
yched CreditAttribution: yched commentedOh damn, this cannot work...
With patch #200 :
- On a D7 where image module was not enabled,
user_update_8011 installs it, and makes sure no image updates will run
then it creates a user_picture field + instance, with 'default_image' in the "new" format (array)
- On a D7 where image module *was* enabled,
user_update_8011 creates a user_picture field + instance, with 'default_image' in the "new" format (array)
image_update_8003 runs and turns $settings['default_image'] into array($settings['default_image']), thus messing with the above.
I think the solution is:
- in user_upadte_8011, set 'default_image' in the "new" (array) format, as in #200:
- in image_update_8003, do
$settings['default_image'] = array($settings['default_image'])
only if $settings['default_image'] is not already an array...Comment #205
slashrsm CreditAttribution: slashrsm commentedHuh... you're right. Let's see what test bot thinks....
Comment #206
yched CreditAttribution: yched commentedFingers crossed :-)
I guess this would warrant a code comment in image_update_8003() about why the settings will already be an array in some cases.
Something like "Other updates, like user_update_8011(), create image fields with the 'default_image' setting directly in the correct format." ?
Comment #207
slashrsm CreditAttribution: slashrsm commentedGood idea. Added. It looks that testbot agrees with us ;)
Comment #208
yched CreditAttribution: yched commentedAlright ! Who will RTBC this then ? :-)
Comment #209
swentel CreditAttribution: swentel commentedIt took me a while to figure out why this was added. Could use a code comment maybe ?
Other than that, looks good to me and the sooner this is in the better so I can start tackling #1953418: Run "Field API : CMI" upgrade as early as possible. and #1969662: Provide new _update_8000_*() CRUD helpers for Field and Instance definitions
Comment #210
swentel CreditAttribution: swentel commentedAdjust title as well a little.
Note: might conflict with #1980058: image_update_8001() should not drop tables - depends who gets in first.
Comment #211
slashrsm CreditAttribution: slashrsm commentedComment that was suggested in #209 added.
Comment #213
slashrsm CreditAttribution: slashrsm commented#211: 625958-file-update-211.patch queued for re-testing.
Comment #214
quicksketchIt looks like a chunk of this patch deals with converting the default value for a file field to become an array. However, only a single value is ever allowed. Why do we need to change the structure of the field to add an unnecessary array? From a data storage perspective, it seems like it would be cleaner code to assume that the default value is not an array with only a single value, i.e. we don't change the default value storage at all. Can we instead modify the handling of the default value where needed?
Comment #215
slashrsm CreditAttribution: slashrsm commentedAgree... I also wanted to do it like this in the first place, but then I figured out it will be a pain to achieve that. I don't remember any more why I felt this way..... It is even possible something changed now with CMI, so it's definitely worth giving this another look.
Comment #216
slashrsm CreditAttribution: slashrsm commentedOK... now I remember. The point here is that you do not have direct control of submit in module that's implementing field. All submit/save logic is done in field_ui and there is not mechanism that would allow you to modify values there.
The only way to achieve this would be to use custom #value_callback which would set value as a single FID instead of an array. The problem with this approach is, that we'd need to massage value before passing it to #default_value when displaying form. Something like this:
This seemed a bit nasty to me and that's why I decided to store array. It just seemed more natural....
I agree that none of this two approaches is 100% clean so I'm fine if we agree on the other approach at the end.
Comment #217
Dave ReidBecause all fields should store their default values in the exact unified way that field values are stored, so using an array for a single value makes files not be a special/odd/bad case, which is good change to make.
Comment #218
yched CreditAttribution: yched commentedNote that "converting 'default_image' setting to an array" was already done in the original patch that was committed in #166.
This followup is just about fixing problems with the how the upgrade function was coded, but does not change what the original upgrade did.
(also, sorry, I couldn't really predict that this followup would take #40 comments to get right, else I would've opened a separate issue :-/)
Comment #219
swentel CreditAttribution: swentel commented#1980058: image_update_8001() should not drop tables also got in, not sure if it conflicts or not.
Comment #220
quicksketchHm, I suppose that might make it "okay", but File fields are already special in that they don't actually set a default value in the form. Instead they are only shown in the event that no files are uploaded at all, so making this an unnecessary array makes it seem even more strange.
Comment #221
chx CreditAttribution: chx commentedRerolled (applies but not cleanly so git apply freaked out), no change and this looks good. The only difference outside of .install is
and this only can be good: always an array. Whether unnecessary or not, I can't judge but having something the same datatype always is good.
As far the update path is concerned, this gets rid of an illegal field_read_fields call in file_update_8002 which is just good and probably makes this critical.
Comment #222
slashrsm CreditAttribution: slashrsm commentedThis patch changes default image configuration back to single value and removes update how which is now not needed anymore.
Comment #223
quicksketchTo follow-up on #220, I don't think holding up this issue is needed. If we decide that putting in additional effort to remove the nested array for the default values, we can pursue it in a dedicated issue.
Comment #224
quicksketchCrosspost.
Comment #225
quicksketchWow, that's great @slashrsm! Thanks for addressing that last lingering concern. This looks the best yet.
The code comment directly above this change is misleading now, since it says it will update the schema to 8002, but it sets it to 8003.
Comment #226
slashrsm CreditAttribution: slashrsm commentedSince update hook was completely removed we can leave this untouched.
Comment #228
chx CreditAttribution: chx commentedUh. I had some trouble decoding that logic... I think this is what you wanted.
Comment #230
slashrsm CreditAttribution: slashrsm commentedThat looks nice. Some tests also need to be fixed.
Comment #232
slashrsm CreditAttribution: slashrsm commented#228 broke UI install. I do not know why. Tried to figured out with no success. This installs locally and should also fix all that nasty exceptions that we got in #226.
Comment #234
chx CreditAttribution: chx commentedDo not try to load raw CMI objects when you have a config entity -- and one that you actually have. The fix for #228-#230 is quite simple, I hope: instead of ->field, we need ->getField().
Comment #235
slashrsm CreditAttribution: slashrsm commentedNice. Thanks for letting me know. We will most likely need another check here.
Comment #237
slashrsm CreditAttribution: slashrsm commentedShould pass now.
Comment #238
tim.plunkettI thought we didn't delete update hooks? See image_update_8001()
This should have a
use Drupal\Core\Entity\EntityInterface;
at the top of the file and just use EntityInterface here.That said, it's a shame we need both entity types here, or we could typehint better. Any reason not to use the proper interfaces here instead of strings? And an elseif?
Comment #239
quicksketchI'm pretty sure we do delete update hooks (though apparently not consistently):
In D8:
system_update_8026
In D7:
system_update_7010
system_update_7019
system_update_7020 - system_update_7026
system_update_7028
system_update_7030
system_update_7031
(many others, got tired of listing)
Comment #240
chx CreditAttribution: chx commentedRemoving update hooks *before beta* is fine. Leaving in them empty is also fine. Neither is a problem. After beta, they are set in stone: not a single line of code in update functions are to be touched. We learned our bitter lesson last release cycle -- one line of code, how bad it could be? It forked the update path and took significant work to unfork it.
Comment #241
chx CreditAttribution: chx commentedRerolled for #238.
Comment #242
tim.plunkettThanks, RTBC if green!
Comment #244
chx CreditAttribution: chx commentedBehold the Bikeshed(TM)! For miniscule details we are holding back from fixing the update path for days stretching into weeks into months (the patch was green end of April). On the other hand, gigantic patches that noone can review properly just fly in and we end up using reflection on normal code execution path. Congratulations.
But, this was RTBC already, the change is not material (wrong Field important) so let's put it back to RTBC and because it's upgrade related, it's deemed now critical.
Comment #245
chx CreditAttribution: chx commentedHere.
Comment #247
chx CreditAttribution: chx commentedif ($type instanceof Field)
sigh.Comment #248
alexpottCommitted 2e63868 and pushed to 8.x. Thanks!
Comment #250
slashrsm CreditAttribution: slashrsm commentedComment #250.0
slashrsm CreditAttribution: slashrsm commentedUpdated summary - UI changes.