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 setting
  • Update 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:

file.png

#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>

CommentFileSizeAuthor
#247 625958_247.patch10.83 KBchx
#247 interdiff.txt795 byteschx
#244 625958_241.patch10.82 KBchx
#244 interdiff.txt438 byteschx
#241 625958_241.patch10.82 KBchx
#237 interdiff.txt746 bytesslashrsm
#237 625958_237.diff10.41 KBslashrsm
#235 interdiff.txt634 bytesslashrsm
#235 625958_235.diff9.86 KBslashrsm
#234 625958_234.patch9.81 KBchx
#232 interdiff.txt1.12 KBslashrsm
#232 625958-file-update-232.patch9.92 KBslashrsm
#230 625958-file-update-230.patch9.81 KBslashrsm
#230 interdiff.txt3.42 KBslashrsm
#228 625958_228.patch6.39 KBchx
#228 interdiff.txt928 byteschx
#226 interdiff.txt1.37 KBslashrsm
#226 625958-file-update-226.patch6.43 KBslashrsm
#222 interdiff.txt5.39 KBslashrsm
#222 625958-file-update-222.patch7.63 KBslashrsm
#221 625958_221.patch5.65 KBchx
#211 625958-file-update-211.patch5.64 KBslashrsm
#211 interdiff.txt568 bytesslashrsm
#207 625958-file-update-205.patch5.41 KBslashrsm
#207 interdiff.txt916 bytesslashrsm
#205 625958-file-update-205.patch5.22 KBslashrsm
#205 interdiff.txt2.45 KBslashrsm
#202 625958-file-update-202.patch5.07 KBslashrsm
#202 interdiff.txt610 bytesslashrsm
#200 interdiff.txt604 bytesslashrsm
#200 625958-file-update-200.patch5.07 KBslashrsm
#198 interdiff.txt705 bytesslashrsm
#198 625958-file-update-198.patch4.66 KBslashrsm
#192 interdiff.txt1.5 KBslashrsm
#192 625958-file-update-192.patch3.97 KBslashrsm
#189 625958-file-update-189.patch4.43 KByched
#189 interdiff.txt1.15 KByched
#187 interdiff.txt2.06 KBandypost
#187 625958-file-update-187.patch4.53 KBandypost
#181 interdiff.txt375 bytesslashrsm
#181 625958-file-update-181.patch3.72 KBslashrsm
#179 interdiff.txt2.09 KBslashrsm
#179 625958-file-update-179.patch3.35 KBslashrsm
#177 interdiff.txt3.9 KBandypost
#177 625958-file-update-177.patch3.29 KBandypost
#174 file_update_8002-625958-174.patch2.78 KBandypost
#174 interdiff.txt2.08 KBandypost
#172 file_update_8002-625958-172.patch2.71 KByched
#157 interdiff.txt690 bytesslashrsm
#157 multiple_fapi_uploads_625958_157.patch82.1 KBslashrsm
#155 multiple_fapi_uploads_625958_155.patch82.06 KBslashrsm
#153 multiple_fapi_uploads_625958_152.patch82.18 KBslashrsm
#143 interdiff.txt3.16 KBquicksketch
#143 multiple_fapi_uploads_625958_143.patch81.88 KBquicksketch
#139 interdiff.txt13.42 KBquicksketch
#139 multiple_fapi_uploads_625958_140.patch80.93 KBquicksketch
#135 interdiff.txt5.65 KBslashrsm
#135 multiple_fapi_uploads_625958_135.patch74.97 KBslashrsm
#134 11.file-field.png70.09 KBtkoleary
#131 multiple_fapi_uploads_625958_131.patch74.95 KBslashrsm
#130 multiple_fapi_uploads_625958_130.patch74.95 KBslashrsm
#130 interdiff.txt3.9 KBslashrsm
#128 interdiff.txt29.84 KBslashrsm
#128 multiple_fapi_uploads_625958_128.patch73.9 KBslashrsm
#124 multiple_fapi_uploads_625958_124.patch94.17 KBslashrsm
#123 multiple_fapi_uploads_625958_123.patch94.13 KBslashrsm
#119 multiple_fapi_uploads_625958_119.patch94.13 KBslashrsm
#119 interdiff.txt3.84 KBslashrsm
#115 multiple_fapi_uploads_625958_115.patch94.44 KBslashrsm
#115 interdiff.txt628 bytesslashrsm
#107 interdiff.txt5.98 KBslashrsm
#107 multiple_fapi_uploads_625958_106.patch94.43 KBslashrsm
#102 interdiff.txt1.79 KBslashrsm
#102 multiple_fapi_uploads_625958_102.patch91.24 KBslashrsm
#98 multiple_fapi_uploads_625958_98.patch91.31 KBslashrsm
#97 multiple_fapi_uploads_625958_97.patch0 bytesslashrsm
#96 error_message.png28.59 KBchaloum
#92 collapsed_fieldgroup_with_content.png4.76 KBtsvenson
#91 multiple_fapi_uploads_625958_91.patch91.32 KBslashrsm
#91 interdiff.txt2.42 KBslashrsm
#88 multiple_fapi_uploads_625958_88.patch90.13 KBslashrsm
#88 interdiff.txt1.34 KBslashrsm
#81 error.png81.08 KBslashrsm
#75 multiple_fapi_uploads_625958_75.patch88.79 KBslashrsm
#75 interdiff.txt2.55 KBslashrsm
#74 multiple_fapi_uploads_625958_74.patch88.13 KBslashrsm
#74 interdiff.txt10.83 KBslashrsm
#72 multiple_fapi_uploads_625958_72.patch77.75 KBslashrsm
#72 interdiff.txt31.03 KBslashrsm
#65 interdiff.txt5.9 KBslashrsm
#65 multiple_fapi_uploads_625958_65.patch58.05 KBslashrsm
#64 interdiff.txt8.52 KBslashrsm
#64 multiple_fapi_uploads_625958_64.patch54.69 KBslashrsm
#63 interdiff.txt21.52 KBslashrsm
#63 multiple_fapi_uploads_625958_63.patch48.3 KBslashrsm
#61 multiple_fapi_uploads_625958_61.patch34.1 KBslashrsm
#55 file.png56.26 KBslashrsm
#55 multiple_upoad_support_625958_55.patch23.26 KBslashrsm
#49 multi_managed_file.png14.61 KBslashrsm
#46 mfw_20120814.patch11.46 KBczigor
#43 core-multiupload_filefield-625958-43.patch25.27 KBdanlinn
#42 reroll-40.patch13.98 KBdroplet
#40 multiupload_imagefield_widget.png129.35 KBgrendzy
#38 625958-38-file_multiple-eojthebrave.patch18.55 KBeojthebrave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Thanks 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.

rickvug’s picture

Project: FileField » Drupal core
Version: 6.x-3.x-dev » 8.x-dev
Component: Code » file.module

Moving 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.

webchick’s picture

This 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

eojthebrave’s picture

I would love to see this happen / Subscribe.

will_in_wi’s picture

subscribe

Deciphered’s picture

Subscribing.

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.

GrimSage’s picture

google 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

philbar’s picture

Version: 8.x-dev » 7.x-dev

Is 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>

will_in_wi’s picture

You also have to handle the files on the server side. See comment #1.

Deciphered’s picture

Version: 7.x-dev » 8.x-dev

Yes, there is a lot more involved than the change in #8.

portait’s picture

Deciphered’s picture

@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.

rickvug’s picture

Today 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).

3dloco’s picture

+1

mgifford’s picture

Issue tags: +html5, +attach files

tagging

mattyoung’s picture

~

droplet’s picture

+1

Ken Hawkins’s picture

So 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"

xandeadx’s picture

+1 for feature request.

Look module "Multiupload Filefield Widget" http://drupal.org/project/1115362

z7’s picture

I 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.

kevinquillen’s picture

#21 HEAD is missing, cannot checkout.

Jacine’s picture

Anyone willing to work on this? Would be nice to have it for Drupal 8.

z7’s picture

#22 Use branch 7.x-1.x (instead of trunk).
Source code is also available at https://github.com/z7/Drupal-module-html5_upload

grendzy’s picture

Title: Support Uploading Multiple Files for HTML5 Browsers » Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute

I 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

Jacine’s picture

Awesome. Thanks @grendzy!

sun’s picture

Next 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.

andypost’s picture

Suppose this feature should live in contrib same as about large file upload widgets

grendzy’s picture

The 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:

  • It fits with the goals of the HTML5 initiative
  • <input type="file" multiple> is becoming widely adopted (e.g. GMail), so users are starting to expect forms to work that way
  • While the existing contrib module has worked well for me, it largely duplicates existing core code (the project contains several comments like "Mostly copied from file.module...)
  • Contrib authors are more likely to support multiple uploads if it's a core feature.
Jacine’s picture

Totally 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.

czigor’s picture

Hi
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?

Jacine’s picture

@czigor Awesome! Patch away! The worst that can happen is that patch doesn't make it, but quite a few of us want it. :)

Dave Reid’s picture

Issue tags: +Media Initiative

I 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!

aspilicious’s picture

This one is a nice one :), please patch!

czigor’s picture

I 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.

yuriy.babenko’s picture

subscribing

czigor’s picture

Assigned: Unassigned » czigor
eojthebrave’s picture

I 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.

sun’s picture

Assigned: czigor » Unassigned
Status: Active » Needs work

The questions raised by @eojthebrave are exactly those I hinted at in #27 already ;)

grendzy’s picture

eojthebrave 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.

czigor’s picture

The 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.

droplet’s picture

Issue tags: +JavaScript
FileSize
13.98 KB

http://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.

danlinn’s picture

Status: Needs work » Needs review
FileSize
25.27 KB

Hello. 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.

Status: Needs review » Needs work

The last submitted patch, core-multiupload_filefield-625958-43.patch, failed testing.

Dave Reid’s picture

Issue tags: +sprint
czigor’s picture

FileSize
11.46 KB

This 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.

slashrsm’s picture

I 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.

eojthebrave’s picture

I 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)

slashrsm’s picture

FileSize
14.61 KB

I see two solutions for update of managed_file element:

  • we keep just one remove button, which would remove all files. That would be similar to the current behaviour with one file.
  • we allow per-file removes and keep upload input on place. If some files are added, we just add them to the list.

I think that the latter is better from UX perspective, but is also a bit trickier to implement. UI could look something like this:

multi_managed_file.png

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.

czigor’s picture

Solution 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.

slashrsm’s picture

First 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.

czigor’s picture

http://drupal.org/project/multiupload_filefield_widget contains the Form API changes too. Just have a look.

slashrsm’s picture

As 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?

czigor’s picture

It 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.

slashrsm’s picture

You 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:

file.png

Some things should be taken into consideration when testing this:

  • this is working prototype, not a finished patch - I'd like to get some feedback before proceeding
  • I was trying to preserve old behaviour for single uploads for backward compatibility. Anyway, '#file_value_callbacks' calls are currently changed a bit, so some things could be broken. This should be easy to fix, though.
  • The easiest way to test this is to enable file_module_test.module and to navigate to file/test.
Deciphered’s picture

The 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.

slashrsm’s picture

Drag and drop ordering is QL. I do not see any reason why we wouldn't add that.

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....

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).

Otherwise, this will potentially break a lot of contrib modules and standard use cases.

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.

Deciphered’s picture

I 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.

slashrsm’s picture

As 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.

czigor’s picture

Sorry, I was a bit slow to get this, thanks for the clarification!

slashrsm’s picture

Assigned: Unassigned » slashrsm
FileSize
34.1 KB

Finally 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.

slashrsm’s picture

Issue summary: View changes

Create summary using standard template.

slashrsm’s picture

Issue summary: View changes

Correctly format UL.

eojthebrave’s picture

I 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.

slashrsm’s picture

@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).

slashrsm’s picture

Issue summary: View changes

Added UI changes part.

slashrsm’s picture

I 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.

slashrsm’s picture

Added 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.

slashrsm’s picture

Issue summary: View changes

Updated issue summary.

aspilicious’s picture

Status: Needs work » Needs review

Lets test this

slashrsm’s picture

As issue desc says tests still need to be fixed, so this will definitely fail...

aspilicious’s picture

Notice: Undefined index: fid in image_field_widget_process() (line 301 of core\modules\image\image.field.inc).
Notice: Undefined index: #file in image_field_widget_process() (line 307 of core\modules\image\image.field.inc).

I applied this patch to my d8 sandbox.

Also when uploading an image to the article content type

Notice: Undefined index: fid in theme_image_widget() (line 425 of core\modules\image\image.field.inc).
Notice: Undefined index: fid in image_field_widget_process() (line 301 of core\modules\image\image.field.inc).
Notice: Undefined index: #file in image_field_widget_process() (line 307 of core\modules\image\image.field.inc).
Notice: Undefined index: fid in image_field_widget_process() (line 301 of core\modules\image\image.field.inc).
Notice: Undefined index: #file in image_field_widget_process() (line 307 of core\modules\image\image.field.inc).

Last but not least. Where can I choose the multiple widget in the UI?

slashrsm’s picture

File field should be working now. Moving on to Image field.

Image field is not implemented yet (working on that). File field should work.

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_65.patch, failed testing.

eojthebrave’s picture

I 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!

+++ b/core/includes/file.incundefined
@@ -1023,14 +1023,14 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+ * The files will be added to the {file_managed} table as  temporary files.

There's an extra space in this sentence.

+++ b/core/includes/file.incundefined
@@ -1044,6 +1044,9 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+ * @param $position
+ *   Delta of a file of only one file needs to be saved. All files

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.

+++ b/core/modules/file/file.moduleundefined
@@ -1021,9 +1039,15 @@ function file_managed_file_value(&$element, $input = FALSE, $form_state = NULL)
+      // Load files if the FIDs has changed to confirm they exist.

has should be have

+++ b/core/modules/file/file.moduleundefined
@@ -1061,28 +1089,41 @@ function file_managed_file_validate(&$element, &$form_state) {
+  // Save entire values to storage

missing . at end of line

+++ b/core/modules/file/file.moduleundefined
@@ -1153,13 +1216,25 @@ function file_managed_file_save_upload($element) {
+    // Value callback expects FIDs to be keys.
+    $uploads = array();
+    $files = array_filter($files);
+    foreach ($files as $file) {
+      $uploads[$file->fid] = $file;
+    }
+

You can probably use drupal_map_assoc() here.

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.phpundefined
@@ -186,13 +186,43 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+    // If we just loaded from DB we have to translate value to
+    // multivalue file widgets.

The wrapping could be fixed on this comment.

Powered by Dreditor.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
31.03 KB
77.75 KB

Thank you for your reviews!

I 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.

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...

+++ b/core/modules/file/file.moduleundefined

@@ -1153,13 +1216,25 @@ function file_managed_file_save_upload($element) {
+    // Value callback expects FIDs to be keys.
+    $uploads = array();
+    $files = array_filter($files);
+    foreach ($files as $file) {

+      $uploads[$file->fid] = $file;
+    }
+

You can probably use drupal_map_assoc() here.

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...

slashrsm’s picture

Issue summary: View changes

Update progress

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_72.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
10.83 KB
88.13 KB

Let's see if we fixed those nasty tests....

slashrsm’s picture

Fixed 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.

slashrsm’s picture

Issue summary: View changes

Update changes.

slashrsm’s picture

Issue summary: View changes

Updated issue summary.

slashrsm’s picture

rooby’s picture

Demo works well for my brief test.
The only concern I would have initially is that it really needs upload progress to be user friendly.

mikeytown2’s picture

uploading 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:

An AJAX HTTP request terminated abnormally.
Debugging information follows.
Path: /file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg
StatusText: n/a
ResponseText:[
   -   {
      "command":"insert",
      "method":"replaceWith",
      "selector":null,
      "data":"\nError message\nThe specified file spacer.gif could not be uploaded. Only files with the following extensions are allowed: txt png jpg.\nFilesUpload multiple files at one. Just browse and select more of them (using Ctrl or Shift + click).\n Add a new file \nFiles must be less than 128 MB.Allowed file types: txt png jpg.\n\n\n",
      -"settings":{
         "basePath":"/",
         "scriptPath":"",
         "pathPrefix":"",
         "currentPath":"file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
         -"tableDrag":{
            -"edit-field-files-und--2-table":{
               -"edit-field-files-und--2-weight":{
                  -"1":{
                     "target":"edit-field-files-und--2-weight",
                     "source":"edit-field-files-und--2-weight",
                     "relationship":"sibling",
                     "action":"order",
                     "hidden":true,
                     "limit":0
                  }
               }
            }
         },
         -"file":{
            -"elements":{
               "#edit-field-files-und-0-upload-upload":"txt,png,jpg"
            }
         },
         -"ajax":{
            -"edit-field-files-und-0-upload-button--2":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_0_upload_button",
                  "_triggering_element_value":"Upload"
               }
            }
         }
      }
   },
   -   {
      "command":"settings",
      -"settings":{
         "basePath":"/",
         "scriptPath":"",
         "pathPrefix":"",
         "currentPath":"file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
         -"tableDrag":{
            -"edit-field-files-und--2-table":{
               -"edit-field-files-und--2-weight":{
                  -"1":{
                     "target":"edit-field-files-und--2-weight",
                     "source":"edit-field-files-und--2-weight",
                     "relationship":"sibling",
                     "action":"order",
                     "hidden":true,
                     "limit":0
                  }
               }
            }
         },
         -"file":{
            -"elements":{
               "#edit-field-files-und-0-upload-upload":"txt,png,jpg"
            }
         },
         -"ajax":{
            -"edit-field-files-und-0-upload-button--2":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_0_upload_button",
                  "_triggering_element_value":"Upload"
               }
            }
         }
      },
      "merge":true
   }
]
ReadyState: undefined
mikeytown2’s picture

Uploading 2 blank txt files caused this output


An AJAX HTTP request terminated abnormally.
Debugging information follows.
Path: /file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg
StatusText: n/a
ResponseText: [
   -   {
      "command":"insert",
      "method":"prepend",
      "selector":"head",
      "data":"\n\n",
      "settings":null
   },
   -   {
      "command":"insert",
      "method":"replaceWith",
      "selector":null,
      "data":"\nError message\n \n  Notice: Undefined index: filename in theme_file_widget() (line 643 of core/modules/file/file.field.inc).\n  Notice: Undefined index: #markup in theme_file_widget() (line 643 of core/modules/file/file.field.inc).\n \n\nFilesUpload multiple files at one. Just browse and select more of them (using Ctrl or Shift + click).\n File informationDisplayWeightOperations \n\n  New Text Document (2).txt  (0 bytes) \n Description \nThe description may be used as the label of the link to the file.\n\n\n \n\n\n Weight for new file -3-2-10123\n\n \n  New Text Document.txt  (0 bytes) \n Description \nThe description may be used as the label of the link to the file.\n\n\n \n\n\n Weight for new file -3-2-10123\n\n \n\n\n\n Add a new file \nFiles must be less than 128 MB.Allowed file types: txt png jpg.\n\n\n",
      -"settings":{
         "basePath":"/",
         "scriptPath":"",
         "pathPrefix":"",
         "currentPath":"file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
         -"ajax":{
            -"edit-field-files-und-0-remove-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_0_remove_button",
                  "_triggering_element_value":"Remove"
               }
            },
            -"edit-field-files-und-1-remove-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_1_remove_button",
                  "_triggering_element_value":"Remove"
               }
            },
            -"edit-field-files-und-2-upload-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_2_upload_button",
                  "_triggering_element_value":"Upload"
               }
            }
         },
         -"tableDrag":{
            -"edit-field-files-und--2-table":{
               -"edit-field-files-und--2-weight":{
                  -"1":{
                     "target":"edit-field-files-und--2-weight",
                     "source":"edit-field-files-und--2-weight",
                     "relationship":"sibling",
                     "action":"order",
                     "hidden":true,
                     "limit":0
                  }
               }
            }
         },
         -"file":{
            -"elements":{
               "#edit-field-files-und-2-upload-upload":"txt,png,jpg"
            }
         }
      }
   },
   -   {
      "command":"settings",
      -"settings":{
         "basePath":"/",
         "scriptPath":"",
         "pathPrefix":"",
         "currentPath":"file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
         -"ajax":{
            -"edit-field-files-und-0-remove-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_0_remove_button",
                  "_triggering_element_value":"Remove"
               }
            },
            -"edit-field-files-und-1-remove-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_1_remove_button",
                  "_triggering_element_value":"Remove"
               }
            },
            -"edit-field-files-und-2-upload-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_2_upload_button",
                  "_triggering_element_value":"Upload"
               }
            }
         },
         -"tableDrag":{
            -"edit-field-files-und--2-table":{
               -"edit-field-files-und--2-weight":{
                  -"1":{
                     "target":"edit-field-files-und--2-weight",
                     "source":"edit-field-files-und--2-weight",
                     "relationship":"sibling",
                     "action":"order",
                     "hidden":true,
                     "limit":0
                  }
               }
            }
         },
         -"file":{
            -"elements":{
               "#edit-field-files-und-2-upload-upload":"txt,png,jpg"
            }
         }
      },
      "merge":true
   }
]
ReadyState: undefined
mikeytown2’s picture

Uploading 1 jpg gave this output

An AJAX HTTP request terminated abnormally.
Debugging information follows.
Path: /file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg
StatusText: n/a
ResponseText: [
   -   {
      "command":"insert",
      "method":"prepend",
      "selector":"head",
      "data":"\n\n",
      "settings":null
   },
   -   {
      "command":"insert",
      "method":"replaceWith",
      "selector":null,
      "data":"FilesUpload multiple files at one. Just browse and select more of them (using Ctrl or Shift + click).\n File informationDisplayWeightOperations \n\n  logo.jpg  (9.86 KB) \n Description \nThe description may be used as the label of the link to the file.\n\n\n \n\n\n Weight for new file -2-1012\n\n \n\n\n\n Add a new file \nFiles must be less than 128 MB.Allowed file types: txt png jpg.\n\n\n",
      -"settings":{
         "basePath":"/",
         "scriptPath":"",
         "pathPrefix":"",
         "currentPath":"file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
         -"ajax":{
            -"edit-field-files-und-0-remove-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_0_remove_button",
                  "_triggering_element_value":"Remove"
               }
            },
            -"edit-field-files-und-1-upload-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_1_upload_button",
                  "_triggering_element_value":"Upload"
               }
            }
         },
         -"tableDrag":{
            -"edit-field-files-und--2-table":{
               -"edit-field-files-und--2-weight":{
                  -"1":{
                     "target":"edit-field-files-und--2-weight",
                     "source":"edit-field-files-und--2-weight",
                     "relationship":"sibling",
                     "action":"order",
                     "hidden":true,
                     "limit":0
                  }
               }
            }
         },
         -"file":{
            -"elements":{
               "#edit-field-files-und-1-upload-upload":"txt,png,jpg"
            }
         }
      }
   },
   -   {
      "command":"settings",
      -"settings":{
         "basePath":"/",
         "scriptPath":"",
         "pathPrefix":"",
         "currentPath":"file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
         -"ajax":{
            -"edit-field-files-und-0-remove-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_0_remove_button",
                  "_triggering_element_value":"Remove"
               }
            },
            -"edit-field-files-und-1-upload-button":{
               "wrapper":"edit-field-files-und--2-ajax-wrapper",
               "effect":"fade",
               -"progress":{
                  "type":"throbber",
                  "message":null
               },
               "event":"mousedown",
               "keypress":true,
               "prevent":"click",
               "url":"/file/ajax/field_files/und/form-VO5PYwCsCIxWf6BjetdZzz6iSSkExvCV03I7am6Rjkg",
               -"submit":{
                  "_triggering_element_name":"field_files_und_1_upload_button",
                  "_triggering_element_value":"Upload"
               }
            }
         },
         -"tableDrag":{
            -"edit-field-files-und--2-table":{
               -"edit-field-files-und--2-weight":{
                  -"1":{
                     "target":"edit-field-files-und--2-weight",
                     "source":"edit-field-files-und--2-weight",
                     "relationship":"sibling",
                     "action":"order",
                     "hidden":true,
                     "limit":0
                  }
               }
            }
         },
         -"file":{
            -"elements":{
               "#edit-field-files-und-1-upload-upload":"txt,png,jpg"
            }
         }
      },
      "merge":true
   }
]
ReadyState: undefined
slashrsm’s picture

FileSize
81.08 KB

#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?

mikeytown2’s picture

Opening 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).

slashrsm’s picture

Status: Needs review » Needs work

Inspecting this...

slashrsm’s picture

Status: Needs work » Needs review

Hm.... 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?

mikeytown2’s picture

With 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

slashrsm’s picture

I 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.

mikeytown2’s picture

The backend works correctly. It is a separate issue; one with core. My guess is something like this
JsonResponse::update

slashrsm’s picture

We should probably report this as a separate issue then.

I added default_image setting upgrade patch for image field.

mikeytown2’s picture

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_88.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
91.32 KB

Let's see what test bot says about this...

slashrsm’s picture

Issue summary: View changes

Add demo

slashrsm’s picture

Issue summary: View changes

Updated issue summary.

tsvenson’s picture

@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.

  • After selecting the files and click "Open" in the file requester, I still needed to click Upload to start the upload. Personally I would prefer it started directly and the Upload button changed to "Cancel Upload".
  • The fieldgroup was collapsed again after the upload was done. This is bad since it then is very easy to miss the alt/title fields (which often is not required to fill in). Note that in the setup I tested, the Images fieldgroup was collapsed to start with.
  • When editing existing content, the fieldgroup is again collapsed even when it does containt content.

A great UX improvement to the fieldgroup would be if it also could give a hint about the content of it, such as:
collapsed_fieldgroup_with_content.png
This way I will know if there are any files or not in there without having to expand it.

slashrsm’s picture

Thanks @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.

tsvenson’s picture

@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 :)

droplet’s picture

I created an issue about Drag & Drop upload but closed and point to here.

#1683838: Add HTML5 Drag & Drop upload to Field file

chaloum’s picture

FileSize
28.59 KB

Just did a manual review of the patch but got this errorerrormassage

How to reproduce the error

Using the patch from #91

  1. created a new content type with the file upload set to multi with a max of 5 files. Added the file types that could be uploaded txt,jpg, png
  2. created an new piece of content went to attach 5 png images, the image name had underscores in the names for example jim_jones.png
  3. press upload then the error appears and no files are uploaded

I using a Mac OSX lion Safari. I havent tried the other browsers yet.

slashrsm’s picture

Hm... 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?

slashrsm’s picture

This should be the right patch.....

micheas’s picture

Having 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.

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_98.patch, failed testing.

chaloum’s picture

just manually tested patch #97 on the the current D8 rebase and the multi file field behaved correctly BUT I could only choose one file.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
91.24 KB
1.79 KB

#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!

chaloum’s picture

On 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).

chaloum’s picture

also 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.

slashrsm’s picture

@tsvenson already suggested that in #92 and we created new issue for that in #93. Your suggestions there are very welocome!

tsphethean’s picture

@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!

slashrsm’s picture

Thank 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

tsphethean’s picture

Status: Needs review » Reviewed & tested by the community

That 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.

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/file/file.install
@@ -245,3 +245,32 @@ function file_update_8000() {
+function file_update_8001() {
...
+    $fields = field_read_fields(array('type' => 'image'));
...
+      field_update_field($field);
...
+      $instances = field_read_instances(array('field_name' => $field['field_name']));
...
+        field_update_instance($instance);

We have to use the update helper functions here.

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.php
@@ -186,13 +186,45 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+  public function massageFormValues(array $values, array $form, array &$form_state) {

+++ b/core/modules/image/image.field.inc
@@ -298,16 +298,17 @@ function image_field_is_empty($item, $field) {
 function image_field_widget_process($element, &$form_state, $form) {
...
-  $item['fid'] = $element['fid']['#value'];
+  $item['fids'] = $element['fids']['#value'];

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.

slashrsm’s picture

Thank you for your review!

We have to use the update helper functions here.

Could you point me somewhere where I can find more info about that?

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.

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:

$node->field_file['und'][0] = array(
  0 => array('fids' => 1),
  1 => array('fids' => 2),
  2 => array('fids' => 3),
);

which also does not seem logical as every items stores just one file.

sun’s picture

1) @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.)

slashrsm’s picture

Will 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.

sun’s picture

Well, 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...?

slashrsm’s picture

All 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.

slashrsm’s picture

Here 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?

slashrsm’s picture

Do you need any more info?

eojthebrave’s picture

Status: Needs review » Needs work

This is looking really good. Couple of nit-picky comments on some of the comments and strings below. Keep up the awesome work slashrsm!

+++ b/core/includes/file.incundefined
@@ -1053,181 +1055,203 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
-    return $upload_cache[$source];
+    if (isset($delta)) {
+      return $upload_cache[$source][$delta];
+    }
+    else {
+      return $upload_cache[$source];

Just nit-picking here but this could be shorter.

return isset($delta) ? $upload_cache[$source][$delta] : $upload_cache[$source];

+++ b/core/includes/file.incundefined
@@ -1053,181 +1055,203 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+      // Munge the filename to protect against possible malicious extension hiding

This comment needs to wrap at 80.

+++ b/core/modules/file/file.field.incundefined
@@ -396,6 +396,47 @@ function file_field_widget_value($element, $input = FALSE, $form_state) {
+      t(
+        'Field %field can hold only @max values. Only @count values were uploaded. The following files have been ommited: %list.',

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."

+++ b/core/modules/file/file.field.incundefined
@@ -561,13 +602,24 @@ function file_field_widget_submit($form, &$form_state) {
+  // If there are more files uploaded via same widget, we have to
+  // separate them, as we display each file in it's own widget.

This comment needs to be wrapped, and I think is missing a "the". ... uploaded via the same widget ...

+++ b/core/modules/file/file.moduleundefined
@@ -870,11 +871,15 @@ function file_managed_file_process($element, &$form_state, $form) {
+
+  // This is used some times so let's implode it just once.
+  $parents_prefix = implode('_', $element['#parents']);

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.

+++ b/core/modules/file/file.moduleundefined
@@ -1065,28 +1093,41 @@ function file_managed_file_validate(&$element, &$form_state) {
+  if ($clicked_button != 'remove_button' && !empty($element['fids']['#value'])) {
+    // Was here but I do not see any sense in it anymore. Leaving here for reference if any
+    // strange bugs occur during development of the patch.
+    //$fids = is_array($element['fids']['#value']) ? $element['fids']['#value'] : explode(' ', $element['fids']['#value']);

Looks like this can probably get removed. However if it does need to get left in the comment should be wrapped at 80 characters.

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.phpundefined
@@ -186,13 +186,45 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+    // Since file upload widget now supports uploads of more than one file at
+    // the time it always returns array of fids. We have to translate this to

"more than one file at a time", and "returns an array of fids."

Powered by Dreditor.

Deciphered’s picture

Just nit-picking here but this could be shorter.

return isset($delta) ? $upload_cache[$source][$delta] : $upload_cache[$source];

While 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:

 if (isset($delta)) {
  return $upload_cache[$source][$delta];
}
return $upload_cache[$source];

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.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
3.84 KB
94.13 KB

Comments included in patch.

slashrsm’s picture

Issue tags: +RTBC Feb 18

This was already RTBC and was changed to needs review for no special reason.

Dave Reid’s picture

This wasn't RTBC on Feb 18 though.

renat’s picture

Well, actually it was set to RTBC at #108.

slashrsm’s picture

Reroll against latest 8.x.

slashrsm’s picture

Another re-roll...

mgifford’s picture

That seemed to work for me in simplytest.me. What else needs to happen to get this RTBC?

slashrsm’s picture

Thank 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.

quicksketch’s picture

-
-  // Force the progress indicator for the remove button to be either 'none' or
-  // 'throbber', even if the upload button is using something else.
-  $ajax_settings['progress']['type'] = ($element['#progress_indicator'] == 'none') ? 'none' : 'throbber';
-  $ajax_settings['progress']['message'] = NULL;
-  $ajax_settings['effect'] = 'none';
   $element['remove_button'] = array(
-    '#name' => implode('_', $element['#parents']) . '_remove_button',
+    '#name' => $parents_prefix . '_remove_button',
     '#type' => 'submit',
-    '#value' => t('Remove'),
+    '#value' => $element['#multiple'] ? t('Remove selected') : t('Remove'),
     '#validate' => array(),
     '#submit' => array('file_managed_file_submit'),
     '#limit_validation_errors' => array($element['#parents']),
     '#ajax' => $ajax_settings,
-    '#weight' => -5,
+    '#weight' => 1,
   );
 
-  $element['fid'] = array(
+  // Force the progress indicator for the remove button to be either 'none' or
+  // 'throbber', even if the upload button is using something else.
+  $ajax_settings['progress']['type'] = ($element['#progress_indicator'] == 'none') ? 'none' : 'throbber';
+  $ajax_settings['progress']['message'] = NULL;
+  $ajax_settings['effect'] = 'none';

It 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 image field's default image configuration to the new format.
+*/

"Comvert" => "Convert". Missing leading space on subsequent lines docblock lines.

+  $fids = array_map(
+    function($item) {
+      return (int) $item;
+    },
+    $fids
+  );

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:

foreach ($fids as $key => $fid) {
  $fids[$key] = (int) $fid;
}
+function form_process_file($element) {
+  if ($element['#multiple'] == TRUE) {
+    $element['#attributes'] = array('multiple' => 'multiple');
+  }
+  $element['#name'] .= '[]';
+  return $element;
+}

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.

+  // Save attached files to the database.
+  if(count(array_filter($_FILES['files']['name'][$upload_name])) > 0) {

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.

slashrsm’s picture

Thank 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...

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_128.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
74.95 KB

This should fix tests.

slashrsm’s picture

Reroll....

Wim Leers’s picture

Status: Needs review » Needs work

It 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:

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+      $extensions = 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp';

Does this really need to be hardcoded? Shouldn't this at least be retrieved from the config system?

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+    if (!config('system.file')->get('allow_insecure_uploads') && preg_match('/\.(php|pl|py|cgi|asp|js)(\.|$)/i', $file->filename) && (substr($file->filename, -4) != '.txt')) {

Why are we explicitly checking certain scripting language extensions here? Why are they hardcoded?

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+      // The .txt extension may not be in the allowed list of extensions. We have
+      // to add it here or else the file upload will fail.
+      if (!empty($extensions)) {
+        $validators['file_validate_extensions'][0] .= ' txt';
+        drupal_set_message(t('For security reasons, your upload has been renamed to %filename.', array('%filename' => $file->filename)));

It feels strange that we're dynamically overriding settings here.

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+    // A URI may already have a trailing slash or look like "public://".

(nitpick) s/URI/file URI/

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+    // If file_destination() returns FALSE then $replace == FILE_EXISTS_ERROR and

s/==/===/, because that's what the if-test tests.

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+    if ($replace == FILE_EXISTS_REPLACE) {

s/==/===/ ?

+++ b/core/includes/form.incundefined
@@ -4612,6 +4612,17 @@ function form_pre_render_file($element) {
+  if ($element['#multiple'] == TRUE) {

s/==/===/ ?

+++ b/core/modules/file/file.field.incundefined
@@ -397,6 +397,47 @@ function file_field_widget_value($element, $input = FALSE, $form_state) {
+ * Validation callback for upload element on file widget. Checks if
+ * user has uploaded more files than allowed.

"user has" should be on the first line.

+++ b/core/modules/file/file.field.incundefined
@@ -562,13 +603,25 @@ function file_field_widget_submit($form, &$form_state) {
+  // If there are more files uploaded via the same widget, we
+  // have to separate them, as we display each file in it's
+  // own widget.

Again, wrong wrapping.

+++ b/core/modules/file/file.field.incundefined
@@ -737,6 +792,14 @@ function theme_file_upload_help($variables) {
+    if ($cardinality == -1) {

s/==/===/
?

+++ b/core/modules/file/file.moduleundefined
@@ -870,11 +871,15 @@ function file_managed_file_process($element, &$form_state, $form) {
+  // This is used some times so let's implode it just once.

s/some times/sometimes/
?

+++ b/core/modules/file/file.moduleundefined
@@ -1065,28 +1090,38 @@ function file_managed_file_validate(&$element, &$form_state) {
+  if ($clicked_button != 'remove_button' && !empty($element['fids']['#value'])) {

s/!=/!==/
?

+++ b/core/modules/file/file.moduleundefined
@@ -1065,28 +1090,38 @@ function file_managed_file_validate(&$element, &$form_state) {
+        if ($file->status == FILE_STATUS_PERMANENT) {

s/==/===/
?

+++ b/core/modules/file/file.moduleundefined
@@ -1065,28 +1090,38 @@ function file_managed_file_validate(&$element, &$form_state) {
+  // Consolidate the array value of this field to array of fids.

Here it's "fids", earlier it was "FIDs".

+++ b/core/modules/file/file.moduleundefined
@@ -1157,13 +1214,24 @@ function file_managed_file_save_upload($element) {
+    $fids = array_map(function($file) {return $file->fid;}, $files);

Closure! :)

I think there need to be spaces around the return statement.

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.phpundefined
@@ -187,13 +187,45 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+    // If we just loaded from DB we have to translate value to multivalue file
+    // widgets.

Needs to be rephrased.

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.phpundefined
@@ -187,13 +187,45 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+    // Since file upload widget now supports uploads of more than one file at
+    // a time it always returns an array of fids. We have to translate this to
+    // a single fid, as field expects single value.

Wrapping needs to be fixed.

tkoleary’s picture

Linking to Usability group seven style guide http://groups.drupal.org/node/283223. There are already some designs around this interaction.

tkoleary’s picture

FileSize
70.09 KB

Image from that discussion:file upload

slashrsm’s picture

Thank 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.

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+      $extensions = 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp';

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).

+++ b/core/includes/file.incundefined
@@ -1049,181 +1051,209 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
+    if (!config('system.file')->get('allow_insecure_uploads') && preg_match('/\.(php|pl|py|cgi|asp|js)(\.|$)/i', $file->filename) && (substr($file->filename, -4) != '.txt')) {

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!

slashrsm’s picture

Status: Needs work » Needs review
quicksketch’s picture

Status: Needs review » Needs work

I 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.

quicksketch’s picture

This patch introduces a strange use of $form_state['storage']:

+  // Save entire values to storage.
+  $values = NestedArray::getValue($form_state['values'], $element['#array_parents']);
+  if (!isset($form_state['storage']['managed_file_values'])) {
+    $form_state['storage']['managed_file_values'] = array();
+  }
+  NestedArray::setValue($form_state['storage']['managed_file_values'], $element['#array_parents'], $values);
+

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.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
80.93 KB
13.42 KB

This 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.

quicksketch’s picture

Re @slashrsm in #135:

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?

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.

jibran’s picture

Wow 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).

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_140.patch, failed testing.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
81.88 KB
3.16 KB

Fixing 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'].

slashrsm’s picture

Thanks 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

Wim Leers’s picture

#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? :)

slashrsm’s picture

I very glad to see interest from you in this. It looks very well and I'm thrilled about that.

What's left here to get this to RTBC? :)

You're the boss. :)

tkoleary’s picture

@slashrsm

Credit goes to Yoroy, Bojhan and ry5n on that design.

tkoleary’s picture

@quicksketch

@tkoleary cross-reference has some additional improvements, but they should certainly be separate from this patch.

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.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

@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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +JavaScript, +html5, +attach files, +sprint, +Media Initiative, +RTBC Feb 18

The last submitted patch, multiple_fapi_uploads_625958_143.patch, failed testing.

andypost’s picture

patch needs re-roll

slashrsm’s picture

Status: Needs work » Needs review
FileSize
82.18 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_152.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
82.06 KB

My bad. Should be fine now.

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_155.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
82.1 KB
690 bytes

Field API CMI patch made _update_7000_field_read_fields() obsolete so using field_read_fields() instead.

Should pass now.

Status: Needs review » Needs work

The last submitted patch, multiple_fapi_uploads_625958_157.patch, failed testing.

moshe weitzman’s picture

Just one fail.

slashrsm’s picture

Status: Needs work » Needs review
slashrsm’s picture

Passes without any problems locally. Trying to retest.

eojthebrave’s picture

Back 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!

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, 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!

slashrsm’s picture

Thank you @eojthebrave and @webchick. Very happy and hoping to see this in soon.

moshe weitzman’s picture

We already support uploading multiple files in same form. IMO, this can qualify as an HTML5 port and thus isn't subject to thresholds.

webchick’s picture

Category: feature » task
Status: Reviewed & tested by the community » Fixed

You know what? I can get behind that.

Committed and pushed to 8.x. Thanks!

GREAT work, all! :D

quicksketch’s picture

LOL. Okay well I won't complain. Ginormous thanks to @slashrsm! Incredible work!

droplet’s picture

Where'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!

Wim Leers’s picture

Awesome work, slashrsm! :) Thank you!

quicksketch’s picture

I 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).

slashrsm’s picture

Wow! 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?

yched’s picture

Status: Fixed » Needs review
FileSize
2.71 KB

A 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.

Status: Needs review » Needs work

The last submitted patch, file_update_8002-625958-172.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
2.78 KB

use Config as object

yched’s picture

Doh, right. Thanks @andypost :-)

slashrsm’s picture

Tested 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?

andypost’s picture

Makes sense!

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-177.patch, failed testing.

slashrsm’s picture

Let's try again.

slashrsm’s picture

Status: Needs work » Needs review
slashrsm’s picture

FileSize
3.72 KB
375 bytes

Found 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.

andypost’s picture

do we have a test about file_managed element? as we need for #table #1975152: Expand render test coverage for #type table

slashrsm’s picture

We 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.

yched’s picture

image_update_8003() needs to be renamed image_update_8001() :-)
[edit: and image_update_dependencies() modified accordingly]

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-181.patch, failed testing.

andypost’s picture

Changing functions order does not help, still 8003 - Moves image module settings from variable to config. @ingroup config_upgrade is pending
It seems that something wrong with update process

andypost’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
2.06 KB

Once update number goes less 8002 - patch show this hack - tests are passes

yched’s picture

Doh, 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 ?

yched’s picture

#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.

andypost’s picture

+++ b/core/modules/image/image.installundefined
@@ -162,19 +174,57 @@ function image_update_8000() {
- */
+ *
 function image_update_8001() {
   db_drop_table('image_styles');
   db_drop_table('image_effects');
-}
+}*/
 
 /**
  * Moves image module settings from variable to config.
  *
  * @ingroup config_upgrade
  */
-function image_update_8002() {
+function image_update_8001() {
+  db_drop_table('image_styles');
+  db_drop_table('image_effects');
   update_variables_to_config('image.settings', array(

This hack allows pass tests but needs work to solve dependency graph anyway

swentel’s picture

Also, we don't drop tables during upgrade, see #1860986: Drop left-over tables from 8.x also, but maybe that's a dedicated issue.

slashrsm’s picture

Hm... 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.

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-192.patch, failed testing.

slashrsm’s picture

I 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.

yched’s picture

[edit: crosspost, #194 is spot on]

yched’s picture

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

Heh, 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 ?

yched’s picture

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 ?

Left 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.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
705 bytes

@yched: thanks for clearing this out!

Let's try....

yched’s picture

The 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.

slashrsm’s picture

FileSize
5.07 KB
604 bytes

Ah... yes. Here it is....

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-200.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
610 bytes
5.07 KB

Try again....

yched’s picture

Oh 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:

'default_image' => !empty($default_image_fid) ? array($default_image_fid) : array(),

- in image_update_8003, do $settings['default_image'] = array($settings['default_image']) only if $settings['default_image'] is not already an array...

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-202.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
5.22 KB

Huh... you're right. Let's see what test bot thinks....

yched’s picture

Fingers 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." ?

slashrsm’s picture

FileSize
916 bytes
5.41 KB

Good idea. Added. It looks that testbot agrees with us ;)

yched’s picture

Alright ! Who will RTBC this then ? :-)

swentel’s picture

+++ b/core/modules/file/file.moduleundefined
@@ -1146,6 +1146,7 @@ function file_managed_file_submit($form, &$form_state) {
+      $fids = array();

It 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

swentel’s picture

Title: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute » [Follow up] Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute

Adjust title as well a little.

Note: might conflict with #1980058: image_update_8001() should not drop tables - depends who gets in first.

slashrsm’s picture

FileSize
568 bytes
5.64 KB

Comment that was suggested in #209 added.

Status: Needs review » Needs work
Issue tags: -JavaScript, -html5, -attach files, -sprint, -Media Initiative, -RTBC Feb 18

The last submitted patch, 625958-file-update-211.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +html5, +attach files, +sprint, +Media Initiative, +RTBC Feb 18

#211: 625958-file-update-211.patch queued for re-testing.

quicksketch’s picture

Status: Needs review » Needs work

It 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?

slashrsm’s picture

Agree... 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.

slashrsm’s picture

OK... 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:

'#default_value' => $field['settings']['default_image']

should be converted to

'#default_value' => array($field['settings']['default_image'])

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.

Dave Reid’s picture

Because 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.

yched’s picture

Note 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 :-/)

swentel’s picture

#1980058: image_update_8001() should not drop tables also got in, not sure if it conflicts or not.

quicksketch’s picture

Because 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.

Hm, 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.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.65 KB

Rerolled (applies but not cleanly so git apply freaked out), no change and this looks good. The only difference outside of .install is

      $fids = array_diff($fids, $remove_fids);
     }
     else {
// this is new
      $fids = array();

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.

slashrsm’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.63 KB
5.39 KB

This patch changes default image configuration back to single value and removes update how which is now not needed anymore.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

To 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.

quicksketch’s picture

Status: Reviewed & tested by the community » Needs review

Crosspost.

quicksketch’s picture

Wow, that's great @slashrsm! Thanks for addressing that last lingering concern. This looks the best yet.

     // Install image.module with schema version 8002 as a previous version
     // would have to create tables that would be removed again.
-    $old_schema = update_module_enable(array('image'), 8002);
+    $old_schema = update_module_enable(array('image'), 8003);

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.

slashrsm’s picture

Since update hook was completely removed we can leave this untouched.

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-226.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
928 bytes
6.39 KB

Uh. I had some trouble decoding that logic... I think this is what you wanted.

Status: Needs review » Needs work

The last submitted patch, 625958_228.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
3.42 KB
9.81 KB

That looks nice. Some tests also need to be fixed.

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-230.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
9.92 KB
1.12 KB

#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.

Status: Needs review » Needs work

The last submitted patch, 625958-file-update-232.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.81 KB

Do 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().

slashrsm’s picture

FileSize
9.86 KB
634 bytes

Nice. Thanks for letting me know. We will most likely need another check here.

Status: Needs review » Needs work

The last submitted patch, 625958_235.diff, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
10.41 KB
746 bytes

Should pass now.

tim.plunkett’s picture

+++ b/core/modules/file/file.installundefined
@@ -259,32 +259,3 @@ function file_update_8001() {
-function file_update_8002() {

I thought we didn't delete update hooks? See image_update_8001()

+++ b/core/modules/image/image.moduleundefined
@@ -1031,3 +1031,26 @@ function image_filter_keyword($value, $current_pixels, $new_pixels) {
+function image_entity_presave(Drupal\Core\Entity\EntityInterface $entity, $type) {

This should have a use Drupal\Core\Entity\EntityInterface; at the top of the file and just use EntityInterface here.

+++ b/core/modules/image/image.moduleundefined
@@ -1031,3 +1031,26 @@ function image_filter_keyword($value, $current_pixels, $new_pixels) {
+function image_entity_presave(Drupal\Core\Entity\EntityInterface $entity, $type) {
+  $field = FALSE;
+  if ($type == 'field_instance') {
+    $field = $entity->getField();
+  }
+  if ($type == 'field_entity') {
+    $field = $entity;
+  }

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?

quicksketch’s picture

I thought we didn't delete update hooks? See image_update_8001()

I'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)

chx’s picture

Removing 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.

chx’s picture

FileSize
10.82 KB

Rerolled for #238.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, RTBC if green!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 625958_241.patch, failed testing.

chx’s picture

Priority: Normal » Critical
Status: Needs work » Reviewed & tested by the community
FileSize
438 bytes
10.82 KB

Behold 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.

chx’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 625958_241.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
795 bytes
10.83 KB

if ($type instanceof Field) sigh.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2e63868 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

slashrsm’s picture

Title: [Follow up] Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute » Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute
Assigned: slashrsm » Unassigned
slashrsm’s picture

Issue summary: View changes

Updated summary - UI changes.