Add cache for media_gallery_block_view()

This looks good, but hard to review since CVS decided to diff the entire thing (not sure why). Can you re-roll so it just diffs the new parts?

Thanks,
J

The patch looks good to me, but in testing I noticed that the link to the images shows a lightbox in the original implementation, and goes to the detail page after applying the patch. The behavior is very consistent - roll back the patch, get a lightbox, apply it again, get the detail page.

Thanks for the work on this. As per #7, postponing until #859602: #cache does not record the #attached declared in child elements lands in core, but @catch, if you want to add tests and re-roll, go for it!

There is no test coverage at all yet for Media Gallery. It would be great to get some, but no one has had the time yet to work on that. I haven't studied either the core patch or this one in depth enough to offer an opinion on whether this particular patch is one worth adding tests to. In any case, it certainly won't hold up this patch being committed once the core one lands, but if anyone is inspired to write tests for any part of Media Gallery, please do!

This patch seems to have caused some issues.

1 is that the block caches, even if I have block caching turned off.

2 Adds the "Add media" link to every block regardless of display settings and as a side effect, makes people add images to the wrong gallery sometimes if they are on the full view for one gallery and the block of another is on the page.

I've rerolled the patch to address concern #2

Thanks catch. If the Media module needs to invoke a new hook somewhere in order to support cache invalidation for modules that render media, like Media Gallery, please feel free to post a patch to the Media queue to do so, link to it from here, and I'll review it.

I reviewed this patch and it looked good to me. It seems to catch all the edge cases I could think of, although as we've seen above this is obviously a pretty scary change :) I haven't actually tested it (and don't have time to right at the moment) so it still needs some testing, but otherwise seems good.

I have also reviewed and commented on the two media issues this depends on:
#1032186: Add hook_media_rendered_flush()
#1034838: media_include_browser_js() should use #attached instead of drupal_add_*

There is one simple problem, though, which is that the patch here was out of date with respect to the name of the hook that was introduced by the media issue linked to above. So, the attached patch changes to use the correct hook name so that it works with the latest patch on that issue. I've attached an interdiff file as well.

By doing $elements['node'] = node_view($node, 'media_gallery_block'); instead $elements += node_view($node, 'media_gallery_block'); I was able to fix the link appearing all the time without having to manually specify _field_extra_fields_pre_render, I think drupal_render() wasn't picking up the pre_render when the node was mixed in the top of the block content array.

It turns out this change has a side effect: It messes up the block's contextual links. We have special code in media_gallery_contextual_links_view_alter() which massages the contextual links, but which no longer runs once this patch is applied.

However, the contextual links were already messed up in a different way to begin with. That is described in #870434: Contextual links can be displayed more than once for the same element (Drupal core issue) but it turns out that core patch has problems of its own, so fixing it in the media gallery is the way to go, and moving the node down a level in the block array is part of the solution for that.

So bottom line is that I wrote a separate patch at #1069750: Contextual links appear twice on the media gallery block which moves it down a level in a way that shouldn't break anything. We either need to incorporate those changes here (or commit that first and reroll this).

Hi, i was trying to implement patch #23.

Got some lines rejected.
I think its because .module was patched before.

Thats an rejected code.

patching file media_gallery.module
Hunk #1 FAILED at 226.
Hunk #2 FAILED at 354.
Hunk #3 succeeded at 503 (offset 10 lines).
Hunk #4 FAILED at 569.
Hunk #5 succeeded at 605 (offset 25 lines).
3 out of 5 hunks FAILED -- saving rejects to file media_gallery.module.rej

***************
*** 226,235 ****
        // @todo Drupal could really use a theme_menu_local_actions() function...
        '#prefix' => '<ul class="field action-links">',
        '#suffix' => '</ul>',
-       // Add front-end resources related to this link, only when it is actually
-       // being rendered.
-       '#pre_render' => array('media_gallery_add_images_link_pre_render'),
      );
  
      // These JS settings are used by the "add media" link but are also shared
      // by the drag-and-drop code below.
--- 226,236 ----
        // @todo Drupal could really use a theme_menu_local_actions() function...
        '#prefix' => '<ul class="field action-links">',
        '#suffix' => '</ul>',
      );
+     // Add front end resources related to this link.
+     $node->content['add_media_link']['#attached']['js'][] = drupal_get_path('module', 'media_gallery') . '/media_gallery.addimage.js';
+     module_load_include('inc', 'media', 'media.browser');
+     media_attach_browser_js($node->content['add_media_link']);
  
      // These JS settings are used by the "add media" link but are also shared
      // by the drag-and-drop code below.
***************
*** 353,369 ****
  }
  
  /**
-  * Prerender function to add front-end resources for the "Add media" link.
-  */
- function media_gallery_add_images_link_pre_render($elements) {
-   drupal_add_library('media', 'media_browser');
-   module_load_include('inc', 'media', 'media.browser');
-   media_attach_browser_js($elements);
-   drupal_add_js(drupal_get_path('module', 'media_gallery') . '/media_gallery.addimage.js');
-   return $elements;
- }
- 
- /**
   * Implements hook_field_extra_fields().
   */
  function media_gallery_field_extra_fields() {
--- 354,359 ----
  }
  
  /**
   * Implements hook_field_extra_fields().
   */
  function media_gallery_field_extra_fields() {
***************
*** 556,604 ****
      $block['content'] = t('No content available.');
    }
    else {
-     // Collect an array of file IDs associated with this gallery. For
-     // simplicity we will assume (here and below) that this is not a
-     // multilingual field. Also note that the node may have been loaded and
-     // viewed elsewhere on the page, in which case the 'media_gallery_media'
-     // field was modified and does not contain what we want, so we have to go
-     // back to the original field value set in hook_node_load() instead, and
-     // also clone the node before changing it so our modifications do not
-     // affect other places where it might be being viewed.
-     $node = clone $node;
-     $node->media_gallery_media = $node->media_gallery_media_original;
-     $files = &$node->media_gallery_media[LANGUAGE_NONE];
-     $gallery_fids = array();
-     foreach ($files as $file) {
-       $gallery_fids[] = _media_gallery_get_media_fid($file);
-     }
-     // Construct a list of file IDs that is limited to the specified number of
-     // items and ordered by most recent; these are the files that will be
-     // displayed in the block.
-     $columns = !empty($node->media_gallery_block_columns[LANGUAGE_NONE][0]['value']) ? $node->media_gallery_block_columns[LANGUAGE_NONE][0]['value'] : 1;
-     $rows = !empty($node->media_gallery_block_rows[LANGUAGE_NONE][0]['value']) ? $node->media_gallery_block_rows[LANGUAGE_NONE][0]['value'] : 1;
-     $number_to_show = $columns * $rows;
-     $block_fids = db_select('file_managed', 'f')
-       ->fields('f', array('fid'))
-       ->condition('fid', $gallery_fids, 'IN')
-       ->orderBy('timestamp', 'DESC')
-       ->range(0, $number_to_show)
-       ->execute()
-       ->fetchCol();
-     // Before sorting, remove any items that will not display in the block.
-     $fid_order = array_flip($block_fids);
-     if ($number_to_show < count($files)) {
-       foreach ($files as $key => $file) {
-         if (!isset($fid_order[_media_gallery_get_media_fid($file)])) {
-           unset($files[$key]);
-         }
-       }
-     }
-     // Prepare the sorting function with the list of file ID orders.
-     _media_gallery_sort_by_recent(NULL, NULL, $fid_order);
-     // Perform the sort.
-     usort($files, '_media_gallery_sort_by_recent');
-     // Display the block.
-     $block['content'] = node_view($node, 'media_gallery_block');
      $block['content']['more_link'] = array(
        '#theme' => 'more_link',
        '#url' => 'node/' . $node->nid,
--- 569,588 ----
      $block['content'] = t('No content available.');
    }
    else {
+     $block['content'] = array(
+       '#node' => $node,
+       '#pre_render' => array('media_gallery_block_build'),
+       '#cache' => array(
+         'keys' => array(
+           'media_gallery_block',
+           $node->nid,
+           (int) node_access('update', $node),
+           (int) $GLOBALS['is_https'],
+         ),
+         'bin' => 'cache_block',
+         'expire' => 86400,
+       ),
+     );
      $block['content']['more_link'] = array(
        '#theme' => 'more_link',
        '#url' => 'node/' . $node->nid,

Would love to see just the list of manual "to do" changes regarding to original code.
I mean like:
line 200 change to this, and line 400 change to that...

Thanks

Could any of you who worked on this please roll a new patch that applies to the current dev?