[D7] Varnish All

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxshafiqissani2126999git

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.

1) please add a link to you project page in the issue summary, see https://drupal.org/node/1011698
2) Those errors from pareview.sh are real, pareview_temp is just the location where your code is checked out.

Hi

I have found some issues when i passed your module into coder
Issues:
Line 65: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_2]

drupal_set_message('Please make sure you have configured the following three variables in the settings.php file.

$conf[\'base_url_varnish_all_all\'] = \'http://example1.com http://example2.com\';

severity: normalreview: i18n_8Line 65: The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable. [i18n_8]

      drupal_set_message('Please make sure you have configured the following three variables in the settings.php file. 
     
$conf[\'base_url_varnish_all_all\'] = \'http://example1.com http://example2.com\';

I think you should pass variables as arguments for security e.g 
drupal_set_message(check_plain(t('Something !var just happened.'), array('!var' => $horrible)),'nuke');


Thanks

I suggest you use a "switch" statement instead of "if" statement:

Now:

if ($form_id === 'varnish_admin_settings_form' && varnish_all_is_configured()) {
$form['varnish_control_terminal']['#access'] = FALSE;
}

if ($form_id === 'system_performance_settings' && varnish_all_is_configured()) {

New:
if (varnish_all_is_configured()) {
switch($form_id) {

case 'varnish_admin_settings_form':
.........................
break;

case 'system_performance_settings':
.........................
break;
}'varnish_admin_settings_form' && varnish_all_is_configured()) {
}

So if first condition case is true you don't execute second case, as was the case with the "if" statement

• Improve your project page.
• Don't use underscore in project name.
• It seems to me that varnish all is not enough explanatory name. Maybe this should even be a patch for varnish or varnish extras or something like that.
• You like to use br everywhere, I don't think that is necessary. Most definitely not in t tags.

@ram4nd: please link to doc pages how something should be improved. For project pages: https://drupal.org/node/997024

Of course under scores are allowed in project names, where did you get that false information?

Actually I don't know about that, I thought about human-readable vs machine-name.

'#suffix' => t('
Enter a link to clear its cache. eg. abc/def/
Leave blank to clear varnish cache for all pages.

'),

Some minor issues:
1. Probably you want to change "cache. eg." into "cache, e.g."
2. Do you really need trailing slash here: abc/def/ ?

Hi shafiqissani,

on line 56 of file varnish_all.module, you pass untranslated HTML code to the function drupal_set_message:

drupal_set_message('<div>Please make sure you have configured the following three variables in the settings.php file. </div> <pre>$conf[\'base_url_varnish_all_all\'] = \'http://example1.com http://example2.com\'; $conf[\'varnish_ip\'] = \'127.0.0.1:6082 127.1.1.2:6082\';</pre>', 'error', FALSE);

You should pass plain (unformatted) text, through the t() function, instead.

HTML tags into translated strings could be a nightmare (think about a translator that missed a closing >).
And also, t() calls should include at least text as possible to avoid translators decide what to translate and what should be translated and what must only be copied.

varnish_all.module (73):

          drupal_set_message(t('<div>Please make sure you have configured the following three variables in the settings.php file. </div><pre>$conf["base_url_varnish_all_all"] = "http://example1.com http://example2.com";
  $conf["varnish_ip"] = "127.0.0.1:6082 127.1.1.2:6082";</pre>'),
            'error', FALSE);

should read

          drupal_set_message(
            '<div>' . t('Please make sure you have configured the following three variables in the settings.php file.') . '</div>'
            . '<pre>$conf["base_url_varnish_all_all"] = "http://example1.com http://example2.com";
            $conf["varnish_ip"] = "127.0.0.1:6082 127.1.1.2:6082";</pre>',
            'error', 
            FALSE
          );

I attached a patch for this...

line 57 should be fixed too...

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2127077

Project 2: https://www.drupal.org/node/2078603

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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