Closed (fixed)
Project:
Scald: Media Management made easy
Version:
7.x-1.x-dev
Component:
Scald core
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
24 Mar 2013 at 06:21 UTC
Updated:
19 Mar 2015 at 20:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
nagy.balint commentedThis is probably related to #1933892: How to best create several atoms at once with a provider
Comment #2
drupa11y commentedO.k. I changed the topic and cool would be a method like e.g. here: http://drupal.org/project/jquery_file_upload


or here: http://drupal.org/project/plupload
Comment #3
nagy.balint commentedI started working on this today because i will need this for a project very soon.
Instead of copying the providers and doing all sorts of "hackish" things, i decided to try and restructure the scald provider hooks to work with multiple atoms created the same time.
I introduced a new provider hook called: hook_scald_add_atom_count
The sole purpose of this hook is to return a number, which is going to be the number of atoms created by scald core (actually scald.pages)
I decided to do this because as i saw from the code the scald.pages global implementation wants to handle atom creation, and the individual provider wants to handle the file section of the form. Therefore it would not be generic if the function creating the atom worked only with a single key (like 'file'), and generally the code would not be clean.
With this new hook the provider can decide how many atoms it needs to be created after an upload, and this can be dinamically determined depending on the form_state. This hook does not need to be implemented though, and if not implemented the value 1 will be used.
I changed the scald_image_scald_add_form_fill hook to get several atoms, so then they can be handled one by one. And also in the form state i store all the atoms instead of just one.
For now on each atom entity type the upload type can be selected. It is on the edit page of the atom bundle. eg: admin/structure/scald/image
If the provider does not use this value then it has no effect. However if the provider needs plupload (or other solutions in the future as this system is probably extendable), then they need to handle the conversion from the value in the form state to a file object.
I created this code for the audio, flash and image providers.
For plupload to work, i used the plupload integration module http://drupal.org/project/plupload. And the plupload javascript library from http://www.plupload.com/
However this module does not handle saving the file in a managed way, so i had to move the files provided by this module, and create the file object for them.
Also since now the second step of the upload process can involve displaying several embedded entity forms, the entity forms had to be separated and they needed different #parents attributes. (also on each field of the atom, as they need to all end up separated in the form_state['values'] as well).
I tested the upload process on images and audio and they worked fine. The only issue i found so far is if you dont start the upload on the plupload widget and just press continue then it bugs out. Which could probably be fixed with some javascript code that disables the continue button until the upload is completed.
Comment #4
nagy.balint commentedAnd a reroll to the current dev as twitter was removed from scald core. And moved the title to the top of the entity forms.
Comment #5
nagy.balint commentedAnd a fix for the scald actions not saving correctly.
Comment #6
nagy.balint commentedI tried to find a way to preserve backwards compatibility with other providers.
And at the end i decided to check if the atom_count hook is implemented, and if it is then the providers gets an array of atoms, a single atom otherwise.
The patch is now smaller and should work with other providers without modification.
Comment #7
aron novakIt would be very cool to have this feature in this module!
I took a look on the code itself and i have a few observations.
1) scald.api.php file is unchanged, but the patch introduces API changes. I think, to make it RTBC, it should be solved.
2) "if you dont start the upload on the plupload widget and just press continue then it bugs out" - shouldn't we include a workaround for this into this patch?
3) i tried to imagine adding other type of upload stuff in addition to normal file upload and plupload. This would mean to change scald.admin.inc, introducing the new type, introducing loose dependency with module_exists. I'm wondering if we can change scald itself in that way, that, for example in plupload or in (a possible) scald_plupload module, with adding a plupload.scald.inc, we can enclose all the upload-type-specific stuff into a single file. Not sure if it's easy to do, but maybe good to consider it as well.
4) Just a little ida that utype should not be an abbreviation maybe.
5) Line 67 of the patch: http://drupal.org/coding-standards#operators ++ should not be separated by a space
Apart from 3), which is maybe overly complex to do, i think this patch is a very good candidate to commit after solving the little things mentioned above
Comment #8
nagy.balint commented1. As I see none of the provider hooks are documented in that file, so i guess it could be a separate issue to create documentation for that.
2. I included a version of this workaround. Put it in the dnd module's js folder cause as the modal window is related to dnd, and this js is related to the modal window, it should be included there.
4. utype was abbreviated on purpose, as the other items are also abbreviated and did not want to change that.
5. fixed.
Also added a class for the atoms in the options form part.
And with the JS workaround i also fixed the other issue, that by default the continue button can be pressed and the options page with the entity can be loaded even when no file was uploaded yet which will result in a broken atom, or a 500 error depending on the provider.
I guess there is still room for improvement.
Comment #9
jcisio commentednagy.balint, thanks! I've just done a quick review and it looks great, particularly it is backward compatible with other atom providers. I think it will get in after we have 1.0 released.
A few remarks, not blocking, that would be nice if we can do before the commit:
1. Try to have every JS file in a library with hook_library, so that there won't be a problem with dependency.
2. While we support only plupload now, with hardcoded condition, we could make it a bit better:
This code implies plupload is the only multiple file upload avaiable. I think we could check $form to see if it is a plupload element.
3. It looks like we don't have a status message with multiple atom creation? Maybe because in that case user usually ignore the atom title? Even though I don't think we have problem displaying a message for each atom.
4. (not trivial one) Is it possible to choose the upload method at the upload time? So that we could have: single file, multiple files, a zip file, fetch from remote url...
Edit: added 4.
Comment #10
nagy.balint commented1. I put the js into the dnd library, but not sure if its the right place.
Also made my javascript more robust by removing the dependency on the 'file' key name.
patch attached.
2. The reason i did that is because only the provider knows why is something in the form. So in this case the provider has to handle the result from the upload. The module_exists plupload is there cause the following part is plupload specific. I guess different uploader tools provide different value structure, so for each of those a separate elseif would need to be implemented in each provider that wants to provide multi upload support. Which of course creates the problem that if we have 4 upload type and a provider only implements 2 of them then the other 2 will not work, even thought it is possible to set it on the UI.
Edit: In the current version i could probably use the $defaults->upload_type in the condition instead of the is_array, as that determines whether we are using plupload or not.
3. I was not sure what to output :) Could have been 'Atoms of type %type has been created'. Also could have just put the message for each uploaded atom. However if someone uploads a lot of atoms like 20+ and all those messages display in the dnd library, it might look bad.
4. Will do some research on this.
Comment #11
drupa11y commentedTHANKS A LOT. You are awesome!
Comment #12
softmax commentedI can confirm that this is a working solution ... thanks to the work and clear documentation of Balint Nagy. Thank you for that sharing.
Comment #13
nagy.balint commented4# At this time the provider constructs the upload portion of the form. So that would mean currently that the provider would need to provide the form elements for all those upload types. And then would need a way to switch which one shows up depending on what upload method the user chooses on the interface.
Maybe a bigger refactor could be done to not require the provider to implement the form elements for all those upload methods, but then maybe the provider will have less freedom in what it outputs.
I think this point could be in a separate issue after this patch is commited.
I rerolled the patch for the latest dev (or latest git commit actually), because there was a minor conflict.
Is there anything that stands between this patch being commited?
Comment #14
jcisio commentedI think it only needs someone to review. I'll try to review it soon. #12 is already a good sign. Also there was an important fix after 1.0, so we might want to release 1.1 immediately then commit this patch to be on the safe side.
Comment #15
jcisio commentedFinally, I had some time tonight so I reviewed, tested and committed it, because it seems that 1.1 is farther away than I thought. Thanks all.
I made some minor changes in coding standards, add a new group in the API file. Let's do the rest in follow-up issues:
- Check and remove the current assumption on multiple uploaded atoms can have different types and remove it if we don't really use.
- Choose upload type directly in the atom creation form.
- Limit of upload type per type/provider.
Change record created: http://drupal.org/node/1993152
Comment #16
jcisio commentedI've removed the JS check for managed_file that prevents the "Continue" button. If we want to check that, we could simply make the input required. Otherwise with that JS check, one can not select a file (without uploading it first) and click Continue.
Comment #17
nagy.balint commentedProbably we should make it required then, as for managed file pressing continue without uploading or selecting any file (assuming the provider uses no extra field to be able to replace the base id) will cause the atom to be broken.
An other solution would be to extend that fix to work like the plupload one, which presses the upload button when the continue button is pressed, and then auto presses the continue button as well.
Comment #18
jcisio commentedEither works for me. However we'll need to think about the behavior change, because without JS, it is possible to Upload, cancel, upload, then Continue (I'm not sure it makes sense, but it is possible BTW).
In any case, thie UX improvement is up for another issue.
Comment #19
gifad commentedyes, I would have requested #16 : providers that need a completed form may do it with "element_validate", else they must be prepared to an empty file.
Two php notices to fix though, at lines 59 & 60 in scald_image module for instance : do nothing if $form_state['values']['file'] is unset.
This has the valuable side effect : continuing with empty file in the "add" step allows to upload the file in the "options" step as well, and brings the opportunity, if filefield_sources and/or imce_filefield modules are installed, to import media files already present on the server (uploaded via ftp or any publishing service).
I've not found any negative side effect, but please can you just confirm that registered atoms can reside in any place on the server, in public;//, but outside of public://atoms/ ?
Thanks
Comment #20
gifad commentedAbout #15 :
There are two important use cases :
I'm working on a solution that begins by providers introducing themselves by, for instance :
scald_pages.inc can then enable the "Source" form if there are multiple providers, or a providers provides such an array.
This "hierarchical" array can be used "as is", and shows up nicely in the form;
Then, scald_pages.inc has to parse the result :
The provider retrieves the upload type via $formstate...
No need of global "Default upload type"
As a side effect, scald core, checking upload type as "managed_file", can skip the (then useless) "Add" step; I'm just diving in Ctools to learn how to do that properly (http://drupal.org/node/1732904#comment-6352530 seems a good hint)
I'm sorry I can't provide a patch(I'm new to git)
Sorry too for so long a post, and my poor english..
Thank's for considering.
Comment #21
gifad commentedI Finally made a patch for this "run-time selection of upload type"
I managed the following issues :
array_map on the hierarchical $sources array crashes when language is not english (problem whith translation of keys ?!) : I wrote a custom translation of values only.
The "bypass useless "Add" step" feature is done only for Image provider : Audio and Flash do necessary operations in hook_scald_add_form_fill() : they have been added a '#required' => TRUE, in hook_scald_add_form
The global "Upload type" has not been removed (yet)...
Note that external providers (scald_file...) work unchanged :)
Comment #22
jcisio commentedHi @gifad, could you create a new issue and post your patch, because the patch of this issue has been committed and it is fixed. I think your patch is big enough to worth a follow-up issue.
Comment #24
henrikakselsen commentedWhat is the status on this issue? #21 patch does not apply cleanly on either dev or stable branch for me.
Comment #25
henrikakselsen commentedI made a new patch that works and applies well for me on current dev branch.
Comment #26
henrikakselsen commentedComment #27
jcisio commentedThe original patch was committed (#15). In the last comment (#22) I asked to file a new issue, with a description of what the new patch does because the bulk upload is already integrated in Scald.
Comment #29
slurpee commentedHello from Drupal's GSoC 2014 team. Google Summer of Code (GSoC)? - an annual program for university students organized by Google with projects managed by open source organization mentors such as us (Drupal!).
We're currently browsing the issue queue looking for projects. Do you think this issue/project is worth a student spending a summer on it and being paid by Google? If so, are you interested in mentoring the student? Learn more about Summer of Code and how to get involved at links below. We're submitting our Summer of Code application to Google in just over 24 hours and looking for last minute ideas. Please respond quickly if you're interested.
Group to join - https://groups.drupal.org/google-summer-code
Ideas for projects for Summer of Code 2014 - https://groups.drupal.org/node/404778
GSoC Homepage http://www.google-melange.com/gsoc/homepage/google/gsoc2014
Google's Summer of Code 2014 Announcement - http://google-opensource.blogspot.com/2014/02/mentoring-organization-app...
Comment #30
nagy.balint commentedThis is an old issue that should be closed.
If any problems, open a new issue please.