Display Cache provides a simple way to cache the html of a specific entity endered in a specific view mode.

The idea is to let each entity handle its own cache independent of external caching strategies. So the cache only refreshes if the displayed entity is actually changed in any way.

Caching the display may have a huge impact on performance issues. See #1996842: Profiling for this module for benchmarks.

Comparision to other caching methods

Panels and Views

Panels and views provide a time-based caching mechanism. You can configure the time to elapse before the cache is rebuilt.

Panels

If a cached pane in Panels is used to display an entity and something is changed on this entity, the user has to wait until the configured caching time is elapsed before he gets the results displayed.

Views

If a cached view is used to display serveral entities and a new entity is added, which should be displayed within the view, the user has to wait also, before he gets the new Views results.

External caching mechanisms

External caching mechanisms like Varnish provide a good caching method unless the project works with logged in users. The request will not be cached bacause of the cookies sent within the call.

Usage

Call the display configuration page, with the display which shall be cached. For example admin/structure/types/manage/article/display/teaser. Enter the display cache section, enable Display Cache and configure the cache granularity.

Project Page: http://drupal.org/sandbox/Caseledde/1970904
Git: git clone http://git.drupal.org/sandbox/Caseledde/1970904.git display_cache

application reviews:
http://drupal.org/node/2000698#comment-7447794
http://drupal.org/node/1970152#comment-7443848
http://drupal.org/node/2001730#comment-7447862

application reviews second run:
http://drupal.org/node/1977172#comment-7467488
http://drupal.org/node/1994016#comment-7467596
http://drupal.org/node/2006076#comment-7472338

application reviews third run:
https://drupal.org/node/2004728#comment-7472520
https://drupal.org/node/2006668#comment-7472626
https://drupal.org/node/2001548#comment-7472750

Comments

PA robot’s picture

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

InternetDevels’s picture

Status: Needs review » Needs work

Hi,
Found some issue:
You have some Error in Codes Sniffer, because name of API file must be .api.php (not .api.inc).

Caseledde’s picture

Status: Needs work » Needs review

Thank you. Fixed.

fmizzell’s picture

I have looked in d.o for a solution to this problem before without luck, so I am fairly certain that there isn't any duplicate modules out there.

I did not run the module through coder (but InternetDevels ran it through Codes Sniffer), but I have been playing with the code for a couple of weeks now, and it is well organized and easy to follow.

The solution itself shows a pretty good understanding of Drupal APIs, and how to work with them (specially entity and field APIs).

I did not do a full functional review of the module, but the basic functionality of the module is working correctly: I went to the manage display tab for one of my content types, set up caching for one of my view modes, and the render array started being cached.

After going through the review checklist, I feel that, if no one else sees any security issues with the code, this project is ready to be promoted to full status.

Caseledde’s picture

Issue tags: +PAreview: review bonus

Added review bonus tag.

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

manual review:

  1. display_cache_install(): do not juggle with module weights as that is very unreliable. Use hook_module_implements_alter() if you have to run before/after a specific other hook.
  2. hook_display_cache_cache_keys_alter(): an actual code example would be nice.
  3. admin_menu/flush-cache/display_cache: this is vulnerable to CSRF exploits. Do not execute any action on a GET request, either use a confirmation form or use a token in the link. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . This is a security blocker. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Otherwise looks pretty good! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Caseledde’s picture

Hi Klausi,

thanks for your review.

1. I can't use hook_module_implements_alter() to alter the weight of hook_module_implements_alter() itself, because of

<?php
if ($hook != 'module_implements_alter') {
  drupal_alter('module_implements', $implementations[$hook], $hook);
}
?>

in module_implements().

See #1997458: It doesn't work!! for more informations why I have to increase the modules weight with hook_install().

2. Working on.

3. You are right with vulnerable to CSRF. But: In my opinion, this is how flush cache via admin_menu works. There are no flush cache callbacks provided by admin_menu with confirmation forms. So this should be a security issue for admin_menu too, shouldn't it?. Now we have three options:

  1. Do nothing: The callback will flush the cache like all other callbacks do it.
  2. Add a confirmation form. Thus the handling is complete different to other cache flushing links. (Bad usability for administrators during development.)
  3. Remove callback. Thus all administrators have to use admin/config/development/display-cache (Bad usability for administrators during development.)

So we have "act as usual" within admin_menu vs. we are aware of CSRF. I really curious about your opinion.

EDIT:

To point 3)
It seems, there are other functions within admin_menu to avoid CSRF. I have to dig a little deeper into admin_menu to get a solution.

Caseledde’s picture

Okay,

I am back,

everything done.

Caseledde’s picture

Issue summary: View changes

Add application reviews

Caseledde’s picture

Status: Needs work » Needs review

Set to needs review.

Caseledde’s picture

Issue tags: +PAreview: review bonus

Added review bonus tag.

klausi’s picture

Assigned: Unassigned » klausi
klausi’s picture

Assigned: klausi » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Review of the 7.x-1.x branch:

  • DrupalPractice has found some issues with your code, but could be false positives.
    
    FILE: /home/klausi/pareview_temp/display_cache.admin.inc
    --------------------------------------------------------------------------------
    FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
    --------------------------------------------------------------------------------
     151 | WARNING | Variable $form_states is undefined.
    --------------------------------------------------------------------------------
    

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. You can use a security token in GET parameters to avoid CSRF, that is what admin_menu does.
  2. display_cache_settings_forms(): why do you need that function? Just use drupal_get_form as page callback in hook_menu()?
  3. "drupal_set_message('Display cache cleared.');": all user facing text must run through t() for translation.
  4. watchdog('display cache', 'The entity type <em>@entity_type</em> s...: use the "%" placeholder instead of "@" which will add the em tags for you.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

klausi’s picture

Issue summary: View changes

Add new application reviews.

Caseledde’s picture

0) $form_states renamed to $form_state. -> Fixed

1) The tokens by admin_menu are already in use. -> Fixed

2) The function was legacy. -> Removed

3) Uses t() now. -> Fixed

4) Uses '%' placeholder. -> Fixed

Caseledde’s picture

Issue tags: +PAreview: review bonus

Added review bonus tag.

patrickd’s picture

Assigned: Unassigned » patrickd
patrickd’s picture

Status: Reviewed & tested by the community » Fixed
  1. define('DISPLAY_CACHE_DISABLED', '0');
    why using a string ('0') instead of the actual integer (0)? (just nitpicking)
  2. display_cache_permission(): 'title' => 'Administer Display Cache',
    user readable text should always be piped through t() (except within hook_menu() implementations, watchdog() calls or database schemas)
  3. you have made the change klausi requested (@entity_type => %entity_type) but you also have to change '@entity_type' => '%entity_type' in the arguments array you pass in watchdog calls. otherwise the string can not be found and will not be replaced
  4. I'd recommend you to write some unit tests.. manually testing this modules functionality will be a bit annoying

Beside that I really liked what I saw,

Thanks for your contribution!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Caseledde’s picture

Thanks a lot for all reviewers.

I just prepare the release.

2. t() is added.
3. I overlooked it. Fixed now.

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

Anonymous’s picture

Issue summary: View changes

Update application reviews.