Page Cache Clear Button

After a very quick look I would say that the code needs to be tidied to meet the coding standards. See:

http://drupal.org/coding-standards

Indents should only be two spaces and if using curly braces they should be like:

if (condition1 || condition2) {
action1;
}

and not

if (condition1 || condition2)
{
action1;
}

The easiest way is to install the coder module which will quickly show any non-standard coding.

I just installed your module in a "virgin" D6 environment, activated the block - noting happens, no block at all!

D6, Windows, Apache, MySQL5, PHP5. Tested as user #1.

Ah. Now I've got it. Ok and sorry for not thinking logically ;)

Walking through your code, I found something else:

In page_cache_clear_button.module, line 85:
db_query("DELETE FROM {cache_content} WHERE cid LIKE '%content:%d%'", $nid);

Clearing the content cache table for node 13 would also clear all nodes with nid 131, 1364438, and so on

Since content entries are always in the form content:nid:vid, you should change that line into
db_query("DELETE FROM {cache_content} WHERE cid LIKE '%content:%d:%'", $nid);

or, probably even better, also use the vid and work without "%" SQL wildcards here.

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/page_cache_clear_button.module:
   +4: [minor] There should be no trailing spaces
   +15: [minor] Comment should be read "Implements hook_foo()."
   +18: [minor] Use an indent of 2 spaces, with no tabs
   +23: [minor] Comment should be read "Implements hook_foo()."
   +33: [normal] Control statements should have one space between the control keyword and opening parenthesis
   +49: [normal] Control statements should have one space between the control keyword and opening parenthesis
   +79: [normal] Control statements should have one space between the control keyword and opening parenthesis
   +93: [normal] Control statements should have one space between the control keyword and opening parenthesis
   +114: [normal] Control statements should have one space between the control keyword and opening parenthesis
   +114: [normal] missing space after comma
   +116: [normal] Control statements should have one space between the control keyword and opening parenthesis
   +133: [normal] curly braces { should end a line, not start one
   +134: [normal] Control statements should have one space between the control keyword and opening parenthesis
  
  Status Messages:
   Coder found 1 projects, 1 files, 9 normal warnings, 4 minor warnings, 0 warnings were flagged to be ignored
  

• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

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

• drupalcs has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...all/modules/pareview_temp/test_candidate/page_cache_clear_button.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   79 | ERROR | Concat operator must be surrounded by spaces
   84 | ERROR | Concat operator must be surrounded by spaces
  --------------------------------------------------------------------------------
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./README.txt ./page_cache_clear_button.info ./page_cache_clear_button.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

both were in sections of code I'd copied from Drupal core

Yes, especially the D6 core would never ever pass any review based on current coding standards ;)

No more issues in the automated review.

Manual review:

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.
• Just to understand it: Why do you want to override the block visibility settings with some admin permissions that are only related to the block? I could limit access to the block just the standard way (in the block configuration), why would I want to set additional role based permissions? As you kind of re-implement something that already exists without no obvious benefit, what am I missing?

Hi,

thanks for clarifying. Here are some few remarks of the latest review scripts:

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...all/modules/pareview_temp/test_candidate/page_cache_clear_button.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   15 | ERROR | Space after opening parenthesis of function call prohibited
   15 | ERROR | Space before closing parenthesis of function call prohibited
   67 | ERROR | String concat is not required here; use a single string instead
  --------------------------------------------------------------------------------
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./README.txt ./page_cache_clear_button.info ./page_cache_clear_button.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Regarding newlines, note my comment: http://drupal.org/coding-standards#comment-5262644

hth, dave

I feel your pain... :-) As someone with 25+ years of programming experience, I also have my own PHP coding style, which usually is not that far from the Drupal standards, actually, quite close in important issues while I also consider some minor requirements pretty moot. For instance, the 80-char line length limit in files when today's monitors usually show 250 or more in a line is very aged and really can't be justified any more.

But you have to view the process in its entirety. First, reviewers are scarce. Many automated tests are copied into the issue queues by fellow applicants waiting for their approval themselves. We have the right to comment on other people's projects, run the git through the automaton but not to decide in deeper code issues or about the overall worthiness of the project. We might have the knowledge but not the right. There are very different kinds of people waiting for their approval here, some of us have dozens of modules behind our backs, long experience in programming, only never needed to apply for contrib rights before. Others might really be fighting with the very first Drupal (or even PHP) module of their lives. The process still has to accommodate all of us.

Besides, there is no straight documentation on how to get a project approved. There are various bits in various places, but not a single description from git or contents of .info files to the automated code reviewers. The veteran reviewers only could tell us why but I have an inkling that this isn't purely due to lack of somebody collecting and writing it. It seems to me more like an intentional hurdle: collecting all pieces of information and presenting our test projects in the first place must be the first grade.

Still, there is one point that should be worked upon but, as I understand, it is fairly new in the review process, that's why it's not really advertised yet in the What to Do for Approval descriptions: these automated test tools. It would really spare a lot of grief if applicants could be sent to Ventral in the first place (provided the infrastructure is sufficient to cope with it). Let them test their projects and apply for approval when the code comes out clean. Let them know up front that this will be the entry test, so there is no point in presenting their projects before the test script returns a blank list of problems.

But as to the actual issues pointed out, we know we don't always agree with every point. Also, there are occasional glitches in the tools themselves, turning up an error where there isn't any. Still, this is something like a university exam. We might do things slightly differently later on but still we have to show that we can adapt to requirements and play by the rules, at least once...

I would say that the challenge is to get the automated tools working on your local system - that way you can fully test your code yourself and make sure there are absolutely no errors in your code.

There are basically three tools which should be used to review your code.

1. Install the modules coder and coder_tough_love. These tests can then be run by going to the module list and selecting 'Code review'.

2. Install pareview_sh module - then it should be possible to get the review running with

sites/all/modules/pareview_sh> ./pareview.sh ../data_export_import 6.x-1.x

The output is in HTML format but you should be able to pipe it into a file for easy review via a browser etc.

3. Install phpcs - this does not need to bootstrap a drupal instance - so can be used from the command line on a locally downloaded copy of the module.

phpcs --standard=DrupalCodingStandard --extensions=php,module,inc,install,test,profile,theme ~/tmp/smoothscroll/smoothscroll/

What I'll try to do is to document this a bit more and add it as a comment on the review process.

Also, you should use http://ventral.org/pareview - I'm not sure if this runs the full suite of coder, coder_tough_love, pareview.sh, phpcs - but it's a really useful resource.

I would add that as long as some of the tools assume a Unix environment, this can't really be prescribed. Only those running under Drupal are really viable: after all, everybody developing a module has a Drupal to run and test it on...

I would suggest that Drupal is primarily developed and hosted on the LAMP stack - it can be made to work with Windows but that's the minority.

It would probably be better in the long term to get hold of an Ubuntu disk and work on Drupal from there - possibly as a Vmware session under Windows. Otherwise you may spend the next several years bumping up against the limitations of running Drupal on Windows.

I never host Drupal on Windows for sure, I'm not that much of a masochist... :-) But development is something quite different. For minor things, you simply edit your PHP files and upload to the server. A PHP is a PHP is a PHP, it makes no difference what platform you type it on (OK, there are CRLF troubles with Git but those can be solved). And for more involved development where I prefer local work and not to upload every other second, I always use Windows running in a VM, under Wampserver. The opsystem is Windows but the environment Drupal sees, AMP is perfectly the same, so I never had the slightest problem with it (and although this is the first time I apply for approval, this is not the first module I write, very far from it :-) ).

I do have an Ubuntu in a VM, of course (I'd even work on it as a full install, I started with Linux in 1994 :-), but as I'm also active in desktop publishing and that's about the only major area that's still practically non-existent on Linux, so that's the way it is, I'm stuck with it).

So anyway, what about this "Page Cache Clear Button" module? :)
Looking forward to it.

• There was a problem with the new line check in pareview.sh, there should be only one single \n character at the end of your files. Sorry for that.
• "$description = 'Show this block only for the selected role(s). If you select no roles, the block will not be visible at all.';": all user facing text should run through t() for translation. Do not translate variables when possible.
• page_cache_clear_button_form_block_admin_configure_alter(): why do you need to alter the role permission handling for this block? Why do you even need your own "administer cache clear button" permission? The block won't be displayed anyway if the role does not have the "view cache clear button" permission. Maybe I miss something here.

If you want to use Windows then that's up to you. I was just pointing out that getting the code review tools to run under Windows would be either a PITA or impossible.

Anyway, after much pushing on my part I've helped with the coding reviews which you were finding conflicting and problematic (as I was).

Basically, it is now sort of accepted that if your code passes the review tool at http://www.ventral.org/pareview then since that combines the current automatic tools you code can be considered to be formatted properly.

Adding a "background" section for better context.