Since we can depend on ctools now (via file_entity). Let's use the plugin system to make augmenting media easier and cleaner.

CommentFileSizeAuthor
#14 1289872-14-media-ctools_media_browser_plugins.patch22.07 KBeffulgentsia
#14 1289872-12-14-interdiff.txt4.79 KBeffulgentsia
#12 1289872-12-media-ctools_media_browser_plugins.patch21.82 KBeffulgentsia
#10 1289872-10-media-ctools_media_browser_plugins.patch19.41 KBeffulgentsia
#9 1289872-9-media-ctools_media_browser_plugins.patch16.59 KBeffulgentsia
#7 1289872-7-media-ctools_media_browser_plugins.patch16.53 KBbecw
#6 1289872-6-media-ctools_media_browser_plugins.patch0 bytesbecw
#2 1289872-2-media-ctools_media_browser_plugins.patch12.83 KBbecw
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

becw’s picture

becw CreditAttribution: becw commented
Assigned: Unassigned » becw

I'm going to work on this issue in a few minutes.

becw’s picture

becw CreditAttribution: becw commented
Assigned: becw » effulgentsia
Status: Active » Needs review
FileSize
1289872-2-media-ctools_media_browser_plugins.patch12.83 KB

Here's a patch that provides a CTools plugin type for media browser plugins. This affects #1224766: Remove default media browser and replace with a default view.

I'm not sure that this is the best way to implement a CTools plugin system. The plugins are classes, but are pretty limited. Given that this is mixing with FAPI/Render API, it might make sense to expect some sort of process method on the plugin class that gets automatically called if present, or to use procedural CTools plugins so that the plugin files are not a class + process function. Anyway, it's a starting point.

Status: Needs review » Needs work

The last submitted patch, 1289872-2-media-ctools_media_browser_plugins.patch, failed testing.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented 
+++ b/includes/media.browser.incundefined
@@ -1,6 +1,47 @@
+class media_browser_plugin {
+
+  // Render the plugin in a media browser.
+  function view($params = array()) {
+    return array('#markup' => '');
+  }
+

We should use studlyCaps

+++ b/includes/media.browser.incundefined
@@ -8,7 +49,18 @@ function media_browser($selected = NULL) {
+  // Build out browser settings. Permissions- and security-related behaviors
...
+  if ($params['multiselect'] == 'false') {
+    $params['multiselect'] = FALSE;

Hmm... I wonder if instead we should do something like
foreach ($params as $k=>$v) {
//convert 'false'/'FALSE' to FALSE and 'true' to TRUE
}

Just to not special case this one. All params should sent 0|1 or true|false.

+++ b/includes/media.browser.incundefined
@@ -29,18 +81,9 @@ function media_browser($selected = NULL) {
+  // Allow plugins to provide a list of enabled or disabled media browser plugins.
   if (!empty($params['enabledPlugins'])) {
     $plugins = array_intersect_key($plugins, array_fill_keys($params['enabledPlugins'], 1));

dunt gettit... allow plugins to provide a list of enabled or disabled plugins? Isnt' that something provided by params?

+++ b/includes/media.browser.incundefined
@@ -48,14 +91,19 @@ function media_browser($selected = NULL) {
+      $plugin_obj = new $plugin_class($plugin);

Should we pass the params to the class constructor and make it a property.

+++ b/includes/media_library.incundefined
@@ -0,0 +1,39 @@
+    $path = drupal_get_path('module', 'media');

silly 2space whitespace on empty line

+++ b/media.moduleundefined
@@ -1014,18 +1014,58 @@ function media_flush_caches() {
+/**
+ * Helper function to get information about all Media CTools plugins of a
+ * particular type.
+ */
+function media_get_plugins($plugin_type) {
+  ctools_include('plugins');
+  return ctools_get_plugins('media', $plugin_type);
+}
+
+/**
+ * Helper function to get information about a single Media CTools plugin.
+ */
+function media_get_plugin($plugin_type, $id) {
+  ctools_include('plugins');
+  return ctools_get_plugins('media', $plugin_type, $id);

Combine these into a single function with id = NULL as a 2nd param?

becw’s picture

becw CreditAttribution: becw commented

Effulgentsia and I also reviewed this approach, and talked about:

  • ctools looks fine
  • put media_browser() back at the top of the page
  • make sure the ctools plugin api version is respected
  • call the old hooks in the new handler::view() methods
  • use a new info hook name (to make current implementations of hook_media_browser_plugin_info() just not do anything, instead of actively break)

These changes will hopefully help decouple the ctools plugin api refactoring from the views in the media browser patch.

becw’s picture

becw CreditAttribution: becw commented
Status: Needs work » Needs review
FileSize
1289872-6-media-ctools_media_browser_plugins.patch0 bytes

Here's a re-roll. I've addressed the comments from #4, and moved some stuff around per #5 (though I completely disregarded our comments on calling the old hooks--it's a pretty straightforward code move, and implementations of the old hook_media_browser_plugin_view() won't be run anyway.

becw’s picture

becw CreditAttribution: becw commented
FileSize
1289872-7-media-ctools_media_browser_plugins.patch16.53 KB

Well, that patch had no bugs, but also no code. Try this one!

Status: Needs review » Needs work

The last submitted patch, 1289872-7-media-ctools_media_browser_plugins.patch, failed testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs work » Needs review
FileSize
1289872-9-media-ctools_media_browser_plugins.patch16.59 KB

Just a reroll for testbot. Still needs review. Working on that.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Assigned: effulgentsia » Unassigned
FileSize
1289872-10-media-ctools_media_browser_plugins.patch19.41 KB

Some minor cleanup plus the addition of the media_internet plugin. This looks good to go to me. Excellent work, becw!

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Assigned: Unassigned » effulgentsia
Status: Needs review » Needs work

Needs a reroll due to #1224766-30: Remove default media browser and replace with a default view being committed. Working on that now.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
1289872-12-media-ctools_media_browser_plugins.patch21.82 KB

Here it is.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Needs work 
+++ b/includes/media.browser.incundefined
@@ -2,13 +2,30 @@
+    if ($v == 'true') { $params[$k] = TRUE; }
+    elseif ($v == 'false') { $params[$k] = FALSE; }
+  }

Should we be using === here? Not sure.

+++ b/media.api.phpundefined
@@ -2,73 +2,35 @@
+ * @code
+ * $plugins['media_upload'] = array(
+ *   'handler' => 'MediaBrowserUpload',
+ *   'title' => 'Upload',
+ *   'access callback' => 'media_access',
+ *   'access arguments' => array('edit'),
  * );
- *

+ * return $plugins;
+ * @endcode

Shouldn't we put this example inside the hook function body as an example?

+++ b/modules/media_internet/media_internet.moduleundefined
@@ -1,22 +1,50 @@
+    'access arguments' => array('edit'),

Little confused as to why we're using 'edit' as an access argument here when it's not really being used.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs work » Needs review
FileSize
1289872-12-14-interdiff.txt4.79 KB
1289872-14-media-ctools_media_browser_plugins.patch22.07 KB

With #13 feedback. Here's the patch, and also a diff between the #12 patch and this one.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Fixed

The only problem I ran into was that the 'Select which plugins are enabled for this field' wasn't adjusted to use the new hook. It was an easy fix so I'm committing this to 7.x-2.x:
http://drupalcode.org/project/media.git/commit/54822b1

AWESOME JOB!

Status: Fixed » Closed (fixed)

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

drzraf’s picture

drzraf CreditAttribution: drzraf commented
Status: Closed (fixed) » Active

reopening after git annotating and found that this patch, commited as 54822b195 introduced this hunk.

function hook_media_browser_plugin_info() {
[...]
  access arguments' => array('create files'),
[...]
}

Some facts:

A sample use-case:

  • an admin should be able to add files to a node using media widget (especially library)
  • an anonymous should be able to add files to a node by uploading new files (without accessing the library)

Consequence:

I believe create files is not granular enough as it applies to creating files both in the node-creation context and separately.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Closed (fixed)

Please please follow issue queue etiquette and do not open long-closed issues. That permission was *not* added in this issue, so please file a separate, new bug report if you must.

drzraf’s picture

drzraf CreditAttribution: drzraf commented

that permission was *not* added in this issue

but that permission started being used when this commit was pushed:

+  $plugins['media_upload'] = array(
+    'title' => t('Upload'),
+    'handler' => 'MediaBrowserUpload',
+    'weight' => -10,
+    'access callback' => 'user_access',
+    'access arguments' => array('create files'),
+  );
+  return $plugins;