Comments

JacobSingh’s picture

Project: D7 Media » File Entity (fieldable files)
JacobSingh’s picture

Status: Active » Needs review
StatusFileSize
new13.59 KB
JacobSingh’s picture

StatusFileSize
new12.19 KB

oops, bad diff.

dave reid’s picture

Status: Needs review » Needs work
+++ b/file_entity.admin.incundefined
@@ -6,6 +6,186 @@
+  $options = array();
+  foreach (module_invoke_all('file_operations') as $operation => $array) {
+    $options[$operation] = $array['label'];
+  }

We should probably add a file_get_operation_info() function that runs the module_invoke_all() and then a drupal_alter() so we only have to call that rather than invoke/alter each time. This function should go in file_entity.module.

+++ b/file_entity.admin.incundefined
@@ -6,6 +6,186 @@
+    '#default_value' => 'approve',

Default value is approve but we don't have an approve operation.

+++ b/file_entity.admin.incundefined
@@ -6,6 +6,186 @@
+  foreach (array_keys(media_get_hidden_stream_wrappers()) as $name) {
+    $query->condition('f.uri', $name . '%', 'NOT LIKE');
+  }

Don't think we can use media functions here.

+++ b/file_entity.admin.incundefined
@@ -6,6 +6,186 @@
+    '#empty' => t('No media available.'),
+    '#attributes' => array('class' => array('media-display-table', 'media-clear')),

Should be 'No files available.' and use 'file' in CSS classes.

+++ b/file_entity.admin.incundefined
@@ -6,6 +6,186 @@
+  $operations = module_invoke_all('file_operations');
+  $operation = $operations[$form_state['values']['operation']];

Rather than re-invoking a hook, let's store the operations in $form['#operations'].

+++ b/file_entity.admin.incundefined
@@ -6,6 +6,186 @@
+  foreach (array_keys($files) as $fid) {
+    $title = db_query('SELECT filename FROM {file_managed} WHERE fid = :fid', array(':fid' => $fid))->fetchField();
+    $form['files'][$fid] = array(
+      '#type' => 'value',
+      '#value' => $fid,
+    );
+    $form['file_titles']['#items'][] = check_plain($title);
+  }

Let's use $files = file_load_mulitple(array_keys($files)) since entities can be cached.

+++ b/file_entity.admin.incundefined
@@ -6,6 +6,186 @@
+    $files = array_keys($form_state['values']['files']);
+    foreach ($files as $fid) {
+      $file = file_load($fid);
+      $files[$fid] = $file;
+      $results[$fid] = file_delete($file);
+    }

Again let's use file_load_multiple() here.

+++ b/file_entity.file_api.incundefined
@@ -487,3 +487,9 @@ function file_uri_to_object($uri, $use_existing = TRUE) {
+function hook_file_operations() {
+  return array(
+    ''
+  );
+}

We should probably provide a valid example, even if it's something silly.

+++ b/file_entity.moduleundefined
@@ -49,6 +49,15 @@ function file_entity_menu() {
+    'access arguments' => array('administer file'),

Is the permission 'administer file' or 'administer files'?

JacobSingh’s picture

Status: Needs work » Needs review
StatusFileSize
new13.06 KB
JacobSingh’s picture

StatusFileSize
new6.67 KB

here's the diff 'o' diffs

ParisLiakos’s picture

Status: Needs review » Needs work

Applied #5
getting this when trying to delete a file in file/%/delete

Notice: Undefined index: file_entity_multiple_delete_confirm in drupal_retrieve_form() (line 738 of /var/www/d7/includes/form.inc).
Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'file_entity_multiple_delete_confirm' not found or invalid function name in drupal_retrieve_form() (line 773 of /var/www/d7/includes/form.inc).
JacobSingh’s picture

Status: Needs work » Needs review
StatusFileSize
new6.46 KB
new13.89 KB

argh... okay, this isn't too pretty, but it works.

ParisLiakos’s picture

Status: Needs review » Needs work

Works!
but a side note:
When you go to file/% and hit delete you are being sent to file/%/delete with no destination.So if you hit cancel,u stay on the same page,if you hit delete you get 404 not found:)

JacobSingh’s picture

Status: Needs work » Needs review
StatusFileSize
new14.09 KB
new1.61 KB

okay, another try :)

dave reid’s picture

Status: Needs review » Needs work
+++ b/file_entity.admin.incundefined
@@ -1,11 +1,131 @@
+  $limit = variable_get('media_admin_limit', 50);

We're using a media variable here. We should just have a default of 50.

+++ b/file_entity.admin.incundefined
@@ -1,11 +1,131 @@
+  $options = array();
+  $form['#operations'] = module_invoke_all('file_operations');
+  foreach ($form['#operations'] as $operation => $array) {
+    $options[$operation] = $array['label'];
+  }

Still no alter hook here.

+++ b/file_entity.admin.incundefined
@@ -1,11 +1,131 @@
+    $options[$file->fid]['operations'] = l(t('edit'), 'file/' . $file->fid . '/edit', array('query' => $destination));

For links of operations, we should really use the standard in node_admin_nodes with theme_links:

$options[$file->fid]['operations'] = array(
'data' => array(
'#theme' => 'links__file_operations',
'#links' => $operations,
'#attributes' => array('class' => array('links', 'inline')),
),
);

+++ b/file_entity.file_api.incundefined
@@ -487,3 +487,26 @@ function file_uri_to_object($uri, $use_existing = TRUE) {
+/**
+ * A list of operations which can be called on files.
+ *
+ * @return
+ *  An associave array of operations keyed by a system name in the following format:
+ *    - label: A string to show in the operations dropdown.
+ *    - callback (string): A callback function to call for the operation.  This function
+          will be passed an array of file_ids which were selected.
+ *    - confirm (boolean): Whether or not this operation requires a confirm form
+ *        In the case where confirm is set to true, callback should be a function
+ *        which can return a confirm form.
+ */

Documentation isn't wrapper properly at 80 characters.

JacobSingh’s picture

Status: Needs work » Needs review
StatusFileSize
new14.09 KB
ParisLiakos’s picture

Status: Needs review » Needs work

Tested again.Cancel link sends you to homepage,delete button deletes and just reloads the page resulting to 404

JacobSingh’s picture

StatusFileSize
new4.34 KB
new14.73 KB

okay, dave's fixes and delete redirect flow + a require_once...

JacobSingh’s picture

Status: Needs work » Needs review
ParisLiakos’s picture

cool,works now:)
but maybe get the cancel link back to file/%/view instead of the homepage?(sorry for this,didnt think that befrore)
but yeah this is working

JacobSingh’s picture

Cool, thanks! I won't post another patch for just that one change, but I agree. as soon as @davereid signs off I'm sending it in!

dave reid’s picture

+++ b/file_entity.admin.incundefined
@@ -1,11 +1,153 @@
+      'author' => theme('username', array('account' => $file)),

theme('username') needs a user account object, so really we should be passing user_load($file->uid) here for 'account'. This can be done as a follow-up.

+++ b/file_entity.moduleundefined
@@ -49,6 +49,20 @@ function file_entity_menu() {
+    'description' => 'Manage files used on your site.',

Default local tasks do not need descriptions - this information is not displayed anywhere.

There is also trailing whitespace in the patch but otherwise if this has been tested when applied and people can delete normally, then this should be good to go. We can have some follow-up cleanups.

JacobSingh’s picture

StatusFileSize
new3.62 KB
new15.3 KB
dave reid’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed this manually and everything looks good in the UI. I have some follow-up issues to address in code but let's get this in.

JacobSingh’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new15.34 KB

last try :)

JacobSingh’s picture

StatusFileSize
new1.9 KB

Interdiff

dave reid’s picture

Status: Needs review » Reviewed & tested by the community
JacobSingh’s picture

Status: Reviewed & tested by the community » Fixed

Finally...

Status: Fixed » Closed (fixed)

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