[D7] Cache Flush

Hi balintcsaba,

I'm really happy to see such a module under develpment
here is a quick review after installing it on a brand new D7

UI/UX
most of below list could be nice addition for better UX, some of them are mistakes
+ admin menu links could be more organized; for ex. separate sub-menu entry for list of existing presets, one for new preset creation
+ add default presets into the list as disabled rows and allow ordering of presets - reflected in them generated menus as well
+ change empty text of table (corrently says "no content")
+ reproduce same (similar) presets as admin menu by default
+ hide save button below form, when nothing to save
+ allow description textfield/area on preset creation, so presets can have an longer explanation
+ add one common label before vertical tabs ("Cache sources")
+ add one general description before vertical tabs ("Select below the different cache sources you wish to clear when your preset is executed. Don't be afraid to select them, all these are flushed when you normally clear all the caches. Select only those you need for better performance. ")
+ vertical tab titles and descriptions:
1.a. Core cache tables
1.b Select any of the cache database tables below, to be truncated when this preset is executed.

2.a. Other core cache options
2.b. Select any of the below functions to be run when this preset is executed.

3.a. Contrib cache tables
3.b. Select any of the tables defined by contributed modules to be flushed when this preset is executed.

4.a. Other contrib cache options
4.b. Note: Some contrib modules have unique ways to store their cache, or to flush them. These require custom configuration, so if you can't find some of your contrib modules here, please submit us an issue on http://drupal.org/project/cacheflush/issues/....
Select any from the list below to clear when this preset is executed.

5.a. Custom (advanced)
5.b. *** copy from the other module ***
5.c. Summary (below label) should say: "X custom rows defined"

+ vertical tab summary when nothing selected: "Nothing selected."
+ cache_block, cache_field, cache_image belongs to core modules, so it should be in the first vertical tab, I guess
+ always have at least one empty row by default in the custom section
+ add "- select table -" default label in the table drop-down in the custom section
+ make preset name field required (show * after label)
+ pre-fill preset name after validation error / refresh
+ go to list of presets after creation
+ arrange width of columns in the preset list
+ Edit | Delete buttons can go into one column called Operations

CODE
+ info file description = "Allows you to create configurable cache flusher presets."
+ I would rename "cacheflush.advanced.preset.inc" into "cacheflush.advanced.inc" or "cacheflush.adv_preset.inc" to be shorter
+ menu item 'admin/cacheflush/clearall' is defined twice in module file
+ add inline comment to describe different parts of the module
+ grammar: _cacheflush_clear_preset_clear_tabel should be *_table, is_tabel should be is_table, You must select a tabel should be table. etc...
+ sometimes you use empty line after function xyz() {, sometimes you don't...

DOCS
+ README.txt is empty

ERROR
+ and finally, when I try to run my custom preset it gives me an error, I assume something is missing from the GIT repo
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE (expire <> '0') AND (expire < '1364248055')' at line 1: DELETE FROM {} WHERE (expire...

Hi,

Nice module. There are some issues I noted.

The menu item 'Cache Flush' is opened in overlay. But after clearing cache you are redirecting to $_SERVER['HTTP_REFERER'].

Please use drupal functions(request_uri()) for getting path. And its better after clearing cache redirect to the overlay page with status message that specifying which preset cache is cleared.

Hi balintcsaba,

I like the idea of this module. Could come in handy when developing. Here's my manual review.

- What's the difference between admin/config/development/cacheflush and admin/config/development/cacheflush/list? It looks like they are exactly the same except for two links.
- I agree with ARUN AK, drupal_goto ($_SERVER['HTTP_REFERER']) is not a nice way of redirecting. I would suggest passing the path where your comming from to _cacheflush_clear_preset(). That way you can drupal_goto where you were.
- typo on line 60 "predafined" cacheflsuh.inc
- When using a wildcard, I'm getting a notice: Notice: Undefined index: wildcard in _cacheflush_clear_preset_clear_table()
- I tried some custom cache presets. Often I end up with a PDO exception:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '0 WHERE (expire <> '0') AND (expire < '1364399600')' at line 1: DELETE FROM {0} WHERE (expire <> :db_condition_placeholder_0) AND (expire < :db_condition_placeholder_1) ; Array ( [:db_condition_placeholder_0] => 0 [:db_condition_placeholder_1] => 1364399600 ) in cache_clear_all() (regel 176 van /includes/cache.inc).

- When I am logged out, and directly navigate to a flush cache preset, for example: admin/cacheflush/clearall. Then I have to login of course. The problem is it keeps redirecting to itself. The browser eventually crashes in in infinate loop of cache clearing:

Error 310 (net::ERR_TOO_MANY_REDIRECTS): There were too many redirects.

Keep up the good work!

Hi balintcsaba,

Thanks for taking the time to make the interface a better experience for the users/developers.
I really like how the module looks and works now.

I see that you added the predefined presets to the list, and even those can be disabled.
It's definitely a good idea. Allowing to order them was just really an extra feature for the future.

There is only one small issue I found:
in the Custom (advanced) vertical tabs section even though I defined more then one lines
it always says 1 custom rows defined in both add and edit sections
As far as I see, only counts rows where wildcard is checked.
If that's fixed, I would mark this as RTBC.

bests,
reszli

Great, not it provides a better feedback. Tx!
It works for me, switching status to RTBC.

I am the owner of the cacheflusher sandbox module, when the applicant contacted me, I suggested we pool our resources and offered him commit access to my repository.

I was in fact willing to cooperate and asked the applicant to instead of duplicating work, contribute to work that had already been done. If by "not cooperating", he means giving him the namespace for the "cacheflusher", then no, I told him we should work together and that I was willing to accept any improvements he might offer or ideas he might have.

Incidentally, I applied a while ago to promote my cacheflusher to full-project status: http://drupal.org/node/1170924

There are over 8000 modules in the Drupal contrib space, which can often lead to new user confusion as they try and figure out which of 5 similar modules is the best one for their use case. As such, duplication of module functionality has become a hot topic within the drupal community; which has adopted a mantra of 'Collaboration vs. Competition'.

The How to Review Full Project Applications contains the following statement:

Module duplication is a real problem on drupal.org, and continues to grow. Again, to ensure that the code on drupal.org is useful to the community of people using Drupal, it is important to ensure that module duplication is avoided.

As the functionality of this module is a complete duplication of functionality provided in 'Devel', I'm afraid it doesn't meet the 'duplicate module' check which is a key component of this review process. In addition, the code doesn't really contain enough substance for a reviewer to get a true sense of your understanding of the Drupal APIs. (I'm not suggesting that you don't understand them; only that it's difficult to confirm your understanding in 35 lines of code.)

However, I certainly do appreciate the motivation for such a module, and that many people would value having this capability without needing to install the full 'Devel' module ... to this end, I would encourage you to consider submitting this as a development code snippet or tip in the 'Module HowTos' section of the Drupal developer's documentation.

Hi bhosmer,

I reviewed your module in question:
(git clone http://git.drupal.org/sandbox/bhosmer/1170266.git cacheflusher)
which simply adds a shortcut to clear the all the caches.

If you carefully read the description of this module, you will see,
that this is a completely different tool, intended for developers and site builders,
allowing fine control over what cache values/tables to be cleared with different presets.

You have to accept, there is not much to pool in with your module here.
If you really wish to help, you can do real applications reviews, instead of bumping the progress.

bests,
reszli

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/cacheflush.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   117 | WARNING | Only string literals should be passed to t() where possible
   118 | WARNING | Only string literals should be passed to t() where possible
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/cacheflush.preset.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   75 | WARNING | Only string literals should be passed to t() where possible
  --------------------------------------------------------------------------------
  
  Time: 1 second, Memory: 14.25Mb
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/cacheflush.preset.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 5 WARNING(S) AFFECTING 5 LINE(S)
  --------------------------------------------------------------------------------
   174 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
   175 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
   178 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
   181 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
   190 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
  --------------------------------------------------------------------------------
  
  Time: 0 seconds, Memory: 5.00Mb
  

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. I could not find a similar module that duplicates this functionality, so I think we can proceed with this application.
2. As coder sniffer says: do no use PHP variables in translatable messages, use placeholders with t() instead.
3. "'#title' => 'Preset List',": all user facing text must run through t() for translation. Please check all your strings.
4. cacheflush_list_preset_form(): do not document $form and $form_state as they are obvious, see http://drupal.org/node/1354#forms
5. cacheflush_list_preset_form(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as preset name I will get a nasty javascript popup on that page. You need to sanitize all user provided text before printing, please read http://drupal.org/node/28984 again.
6. cacheflush.module: why do you include all your files globally? So they are all loaded on every single page request? You should only include your files in functions when you actually need them. You can specify the file to be loaded in hook_menu() for example.
7. _cacheflush_clear_all(): this is vulnerable to CSRF exploits. Do not execute such actions on simple GET requests, either provide confirmation forms or security tokens in the link! See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained
8. _cacheflush_clear_all(): $base_path is unused?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects. I also removed two automated review links from the issue summary. I'm also adding the security tag, please don't remove it, we are using it for statistical purposes and to show examples of security problems.

removed automated reviews.

Hi balintcsaba,

based on klausi's review:

2. & 3. use of t() function
cacheflush.preset.inc line 73 and 90
t() here: '#title' => t("@val", array("@val" => $value)), is redundant
no need to translate a variable only, the $value options were already passed though t() in lines 61-65

4. do not document $form and $form_state
cacheflush.preset.inc lines 263-364
you can also remove this

 * @param array $form
 *   Form element to be displayed.

7. token used incorrectly
instead of

$items["admin/cacheflush/clearall/" . drupal_get_token()] = array(

...

// Check for valid token.
drupal_valid_token($token);

you should use it as

$items["admin/cacheflush/clearall"] = array(
  'title' => 'Clear All',
  ....
  'options' => array('query' => array('token' => drupal_get_token('some_string_ex_cacheflushe'))),
)

...

if (!drupal_valid_token($_GET['token'], 'some_string_ex_cacheflushe')) {
   return drupal_access_denied();
}

9. incorrect sentence on UI
cacheflush.preset.inc line 309
Table ($table): The table $table to delete from.
should be
Table ($table): The name of the table to delete from.

that's it :)
hope you can fix this soon, I'd really like to use this module asap!

reszli

Hi balintcsaba,

sorry to ruin your weekend, but now I have some spare time to do reviews
here are a couple more things to fix after another manual review:

1.
cacheflush.preset.inc
line 73: '#title' => t('@val', array('@val' => $value)),
line 92: '#description' => t("@val", array("@val" => $value['description'])),
incorrect use of t() for a variable only
you should remove t() from these two lines
and add them back to (only) lines 61-65 in the array declaration

2.
cacheflush.advanced.inc line 11
no need to document $form and $form_state
you should document $preset_id and $cache_presets

3.
spelling: Tabel => Table
4 matches in 2 files
cacheflush.inc line 58
cacheflush.advanced.inc lines 20, 93 and 128

4.
cacheflush.admin.inc line 12
global $base_url; not used

5.
cacheflush.admin.inc line 75
'#value' => t('Add rows'),
would be better as
'#value' => t('Add another row'),

6.
it would be a good idea to use a confirmation form on custom preset's "Delete" link

I changed your tags:
"review bonus" should be "PAReview: review bonus"

Hi balintcsaba,

I reviewed all the points mentioned above
one issue I realized when checking point 5.:

you can't check 'clicked_button' by it's '#value' to determine which button was clicked,
since that goes through t() and the string will differ when translated
in cacheflush.advanced.inc
line 26: if (isset($form_state['clicked_button']) && ($form_state['clicked_button']['#value'] == 'Add another row' || $form_state['clicked_button']['#value'] == 'Remove')) {
line 29: if ($form_state['clicked_button']['#value'] == 'Remove') {
line 43: if ($form_state['clicked_button']['#value'] == 'Add another row') {

you do check it correctly by the '#id' in cacheflush.preset.inc
line 132: if ($form_state['clicked_button']['#id'] == 'edit-delete') {

you should do the same in advanced.inc inc as well
other than that, it should be fine now
Tx!

Great! Tx!

Looked ok to me. I can tell this could be usefulI for the following problem. In the past where due to some problems with varnish module. My CSS wouldn't render correctly anymore, so if that cache can be targeted and flushed on a schedule that would be great.

In regards to point 5 on #19, the best way to handle this is to set submit callbacks on the submit buttons (via the button's #submit element). Then the appropriate callback runs when the button is clicked instead of running everything through a generic callback and trying to figure out which button the user clicked.

manual review:

1. cacheflush_menu(): you cannot use drupal_get_token() in hook_menu(), since that information is cached on not executed per user. You need to generate and add the token when you output the link, which means that the link cannot be defined in hook_menu() (as far as I know). Currently the menu entry will only work the one user that triggered the hook_menu() rebuild.
2. _cacheflush_advanced_form(): do not use the raw $form_state['input'], always use $form_state['values '] where possible. Same in cacheflush_preset_form_validate().
3. "confirm_form($form, 'Are you sure?', 'admin/config/development/cacheflush', $desc);": all user facing text must run through t() for translation.

Although you should definitively fix those issues they are not critical application blockers, so ...

Thanks for your contribution, balintcsaba!

I updated your account to let you 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

Bonus update