| Project: | File entity (fieldable files) |
| Version: | 7.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Media Initiative, sprint |
Issue Summary
The current workflow when files are added is like this:
* Add the file (through the add form or media browser widget.
* The file is added, and the entity has been created.
* You can now edit the entity, when you add the entity through the add form, you get to the edit form immediately.
There are some problems with this, primarily since we actually can add files without inputting required fields. I propose that we change this to a two-step process:
1. The file is uploaded through the form, and we refresh the form through AJAX (with fallback of course). We don't store the file entity at this point.
2. Once we know what file we are dealing with, we know which bundle it should be associated with. This means we can attach the proper field form form using the right bundle, and all the normal validation can occur.
Step one needs to be pluggable somehow, since not all uploading methods involve uploading a file, like selecting a youtube video. We also need to store a reference to the file that we just uploaded so that we can delete it (on cron) if the field data is not validated.
Comments
#1
Changing title
#2
Yeah I like the idea of making this a multi-step process that maybe skips itself if there are no fields available. If we store the file entity with $file->status = 0, this means the file is temporary, and we can change $file->status = FILE_PERMANENT when the user completes step 2. System module automatically cleans up and deletes temporary files.
#3
related discussion #1426730: Automatically open the file edit modal after uploading a file in the media module issue queue
#4
Tagging
#5
The patch adds a multi-step wizard for adding files. This wizard now always consists of three steps:
There is only one filetype to choose from until #1292382: Make it possible to create any number of custom File Types has been fixed. Note that the file is saved directly after uploading, without knowing the correct filetype yet. This patch overwrites $file->type before saving. This change is not yet saved in the file object, which will be fixed by #1637382: type of file is overwritten in file_entity_file_presave()..
Todo's:
#6
Patch attached skips steps 2 and 3 if not applicable and integrates with the File Access API.
Required patches before applying this patch:
- http://drupal.org/node/1292382#comment-6122536
- http://drupal.org/node/1227706#comment-6121346
#7
FYI, we had an interesting brainstorming about improving the UX/usability of adding one or multiple files to a Drupal site in Barcelona. Basically we came up with a way of just having one modal screen and a dynamic AJAX driven interface.
It's one of things I got on my now even longer todo-list to type up. Expect it within the next week or so.
#8
Ah, you recovered from the weekend :)
The patch already needs to be changed since the multiple file types thing is also changing. Let me know when you wrote it all down so we can continue the discussion.
#9
Well, sort of recovering. Its easy since meeting so many great people at events like this is really energizing.
I'll post here when I have got to writing it up. Thanks for your great help.
#10
@tsvenson, #7:Hey Thomas! Did you manage to make some time to perhaps file an issue I can follow on that modal screen + dynamic AJAX driven interface you talked about in Barcelona?
#11
@klonos: Not yet. I'm currently working on a bigger document that covers much more. Hope to have a first draft ready in a week or two.
#12
Cross referencing #1678106: Mobile Friendly Media Browser
#13
Rerolled patch in #6 against HEAD. Also, note to testers - run drush updb
#14
I tested the patch in #13 after applying the previous patches. It seemed like there was an issue where if a file that was uploaded had multiple applicable file types, you could select the filetype fine, but if it had any associated fields, the form would skip the entry of those fields. I've attached a modified version of #13's patch to correct this behavior.
#15
The wizard seems to work fine, but renamings in file_entity causes troubles in media module which makes testing a bit harder.
One thing that doesn't work fine is the ctools model popup. The popup doesn't close after finishing the form. I tried removing the redirect and dived into the ctools code, but couldn't get rid of the pesky popup...
Anyway, I've renamed file_access() to file_entity_access() to make this compatible with the latest patches from #1227706: Add a file entity access API. I also changed the label for filetype. There was a "@todo" there to replace it show the label instead of machine name. It's replaced by a call to entity_label().
Note that the access api patch is still a dependency. I used the following patch for testing:
http://drupal.org/node/1227706#comment-6623878
#16
Forgot to mention, the problem with the popup is on the popup used for adding files from the media selector field (on a node edit page), not on the content -> files tab.
#17
Tested patch from #15 using latest media 7.x-2.x-dev. #1227706 is comitted so did not have to apply patch. Remember to update db and clear caches if upgrading Media or File Entity as there are many function changes.
Working great for me. Please note if you are a user who can edit the uploaded file you will be redirected to the edit modal after file upload. This is set by the Media module so if that is an issue it should be opened there.
#18
#19
entity_label() was returning $file->filename, which was a bit confusing. I've hooked a label callback into the entity to help clarify Step 2 of the upload wizard.
#20
The last submitted patch, file_entity-multi_part_form_upload-1054616-19.patch, failed testing.
#21
Ahh, no on 19 - there is an existing implementation of entity_label() that seems to make sense (see
file_entity_edit_submit()). Instead, I moved this back over to type, but this time withfile_entity_type_get_name().#22
#23
The last submitted patch, file_entity-multi_part_form_upload-1054616-21.patch, failed testing.
#24
Ok, setting status to start test.
#25
crossposting..
#26
#21: file_entity-multi_part_form_upload-1054616-21.patch queued for re-testing.
#27
haha mauritsl, I'll stop pressing buttons :) We're in a race condition.
#28
Since this module has a dream of core, I'm thinking we shouldn't extend this wizard to support plupload. Currently, there is no multiple file upload support in this wizard patch. Should we invest in this direction as well? Or... commit this patch for single uploads, and open a new issue for multiples?
#29
There is already a ticket for that: #1599892: Plupload integration broken
I suggest committing this single file upload thing, after we have done a little more testing.
#30
In the interest of testing - I added two test cases with a handful of steps each for this feature.
#31
You're a hero!
I tested adding files using both the file tab and the media browser (wysiwyg plugin). Everything works well.
Only one thing in the media browser.. It redirects to the edit page after a file has been added. That redirect should now be removed, but this is in the media module, not file_entity. I suggest to open an issue there after this has been committed.Tinker already said this in comment 17 :)Marking this patch as RTBC.
#32
Rerolled against recent commit: http://drupalcode.org/project/file_entity.git/commit/11894c1
Also, tossed in a couple minor Coder recommendations and fixed a spelling error.
#33
The last submitted patch, file_entity-multi_part_form_upload-1054616-32.patch, failed testing.
#34
#32: file_entity-multi_part_form_upload-1054616-32.patch queued for re-testing.
#35
There we go!!
#36
Just a quick look
+++ b/file_entity.pages.incundefined@@ -116,45 +190,130 @@ function file_entity_usage_page($file) {
+ // Check fields from Field API.
+ if (db_select('field_config_instance', 'fci')
+ ->fields('fci', array('id'))
+ ->condition('fci.entity_type', 'file')
+ ->condition('fci.bundle', $type)
+ ->condition('fci.deleted', 0)
+ ->range(0, 1)
+ ->execute()
+ ->fetchField()) {
+ return TRUE;
}
+ return FALSE;
any reason why we are not using http://api.drupal.org/api/drupal/modules%21field%21field.info.inc/functi... here?
+++ b/file_entity.pages.incundefined@@ -200,8 +358,8 @@ function file_entity_add_upload_multiple($form, &$form_state, $params = array())
- $form['upload']['#upload_location'] . '/' :
- variable_get('file_default_scheme', 'public') . '://';
+ $form['upload']['#upload_location'] . '/' :
+ variable_get('file_default_scheme', 'public') . '://';
why adding indentation?
+++ b/file_entity.pages.incundefined@@ -411,13 +569,14 @@ function file_entity_delete_form($form, &$form_state, $file) {
- '%title' => entity_label('file', $file),
- )),
- 'file/' . $file->fid,
- $description,
- t('Delete')
+ '%title' => entity_label('file', $file)
+ )),
+ 'file/' . $file->fid,
+ $description,
+ t('Delete')
same..also comma is removed from '%title'
#37
Thanks for the review. I've fixed these coding style errors and use field_info_instances instead. There is no reason for the custom function.
#38
Can confirm that the flow still works good using field_info_instances().
I leave it to someone else to give the RTBC now ;)
#39
#37 applied and passed :)
#40
Non functional review.
+++ b/tests/file_entity.testundefined@@ -291,7 +291,114 @@ class FileEntityTypeTestCase extends FileEntityTestHelper {
+ * Make sure candidates are presented in the case of multiple ¶
+ * file types.
+ */ ¶
+++ b/tests/file_entity.testundefined@@ -291,7 +291,114 @@ class FileEntityTypeTestCase extends FileEntityTestHelper {
+ $user = $this->drupalCreateUser(array('create files'));
+ $this->drupalLogin($user);
+ ¶
+ // Step 1: Upload file
+ $file = reset($this->files['image']);
+ $edit = array();
+++ b/tests/file_entity.testundefined@@ -291,7 +291,114 @@ class FileEntityTypeTestCase extends FileEntityTestHelper {
+ $edit['files[upload]'] = drupal_realpath($file->uri);
+ $this->drupalPost('file/add', $edit, t('Next'));
+ ¶
+ // Step 2: Select file type candidate
+ $this->assertText('Image 1', 'File candidate list item found.');
+++ b/tests/file_entity.testundefined@@ -291,7 +291,114 @@ class FileEntityTypeTestCase extends FileEntityTestHelper {
+ * NOTE: Depends on file_entity.module default 'image' type.
+ */ ¶
+ function testTypeWithoutCandidates() {
+ // Attach a text field to the default image file type.
+++ b/tests/file_entity.testundefined@@ -291,7 +291,114 @@ class FileEntityTypeTestCase extends FileEntityTestHelper {
+ $user = $this->drupalCreateUser(array('create files'));
+ $this->drupalLogin($user);
+ ¶
+ // Step 1: Upload file
+ $file = reset($this->files['image']);
+ $edit = array();
+ $edit['files[upload]'] = drupal_realpath($file->uri);
+ $this->drupalPost('file/add', $edit, t('Next'));
+ ¶
+ // Step 2: Complete field widgets
+ $langcode = LANGUAGE_NONE;
Remove trailing spaces where ¶ symbol appear. Not critical but it should match coding requirements.
#41
Rerolled #37 against #32, passes Coder minor, improved docs, removed trailing commas.
#42
No functional changes, but did a sanity check to see if everything still works.
I can confirm that it does not add minor issues in coder.
#43
This looks good:)
+++ b/file_entity.pages.incundefined@@ -200,8 +342,8 @@ function file_entity_add_upload_multiple($form, &$form_state, $params = array())
- $form['upload']['#upload_location'] . '/' :
- variable_get('file_default_scheme', 'public') . '://';
+ $form['upload']['#upload_location'] . '/' :
+ variable_get('file_default_scheme', 'public') . '://';
Adding indentation again.
+++ b/file_entity.pages.incundefined@@ -403,7 +545,7 @@ function file_entity_delete_form($form, &$form_state, $file) {
- '#value' => $file->fid,
+ '#value' => $file->fid
Removing comma
+++ b/file_entity.pages.incundefined@@ -432,7 +573,7 @@ function file_entity_delete_form_submit($form, &$form_state) {
- '%title' => entity_label('file', $file),
+ '%title' => entity_label('file', $file)
Removing comma
+++ b/file_entity.pages.incundefined@@ -451,7 +592,7 @@ function file_entity_multiple_delete_form($form, &$form_state, array $files) {
- '#tree' => TRUE,
+ '#tree' => TRUE
same
+++ b/file_entity.pages.incundefined@@ -469,13 +610,13 @@ function file_entity_multiple_delete_form($form, &$form_state, array $files) {
- '#suffix' => $title . "</li>\n",
+ '#suffix' => $title . "</li>\n"
same
+++ b/file_entity.pages.incundefined@@ -469,13 +610,13 @@ function file_entity_multiple_delete_form($form, &$form_state, array $files) {
- '#value' => 'delete',
+ '#value' => 'delete'
same
#44
Fixed issues from #43. No functional changes.
#45
Tested patch. Patch is clean and functional
#46
Ok, looks good, has tests, i love it, what more is needed?
I commited this. Could someone open an issue to media queue to remove the edit screen after uploading a file? i guess it is not needed anymore;)
Thanks all!!
#47
#1848034: Remove ctools edit form after uploading a new file in media browser
#48
Automatically closed -- issue fixed for 2 weeks with no activity.
#49
This change doesn't allow the File Name of the new file to be edited in the upload flow; only the "normal" fields are available for editing. Is this an oversight? Editing of the file name was previously supported at the time of upload when this functionality was handled by Media.