Auto archiver

You should do a spellcheck of your project applications intro!

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

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

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. Get a review bonus and we will come back to your application sooner.

FILE: ...n/www/drupal7/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 37 WARNING(S) AFFECTING 38 LINE(S)
--------------------------------------------------------------------------------
 12 | WARNING | Line exceeds 80 characters; contains 131 characters
 13 | WARNING | Line exceeds 80 characters; contains 135 characters
 14 | WARNING | Line exceeds 80 characters; contains 135 characters
 15 | WARNING | Line exceeds 80 characters; contains 134 characters
 16 | WARNING | Line exceeds 80 characters; contains 132 characters
 17 | WARNING | Line exceeds 80 characters; contains 133 characters
 19 | WARNING | Line exceeds 80 characters; contains 91 characters
 21 | WARNING | Line exceeds 80 characters; contains 136 characters
 23 | WARNING | Line exceeds 80 characters; contains 130 characters
 24 | WARNING | Line exceeds 80 characters; contains 102 characters
 25 | WARNING | Line exceeds 80 characters; contains 120 characters
 31 | WARNING | Line exceeds 80 characters; contains 99 characters
 32 | WARNING | Line exceeds 80 characters; contains 93 characters
 34 | WARNING | Line exceeds 80 characters; contains 136 characters
 35 | WARNING | Line exceeds 80 characters; contains 139 characters
 37 | WARNING | Line exceeds 80 characters; contains 125 characters
 38 | WARNING | Line exceeds 80 characters; contains 132 characters
 39 | WARNING | Line exceeds 80 characters; contains 132 characters
 42 | WARNING | Line exceeds 80 characters; contains 109 characters
 44 | WARNING | Line exceeds 80 characters; contains 147 characters
 45 | WARNING | Line exceeds 80 characters; contains 99 characters
 46 | WARNING | Line exceeds 80 characters; contains 136 characters
 48 | WARNING | Line exceeds 80 characters; contains 123 characters
 49 | WARNING | Line exceeds 80 characters; contains 119 characters
 51 | WARNING | Line exceeds 80 characters; contains 141 characters
 52 | WARNING | Line exceeds 80 characters; contains 134 characters
 55 | WARNING | Line exceeds 80 characters; contains 138 characters
 56 | WARNING | Line exceeds 80 characters; contains 85 characters
 58 | WARNING | Line exceeds 80 characters; contains 123 characters
 60 | WARNING | Line exceeds 80 characters; contains 120 characters
 62 | WARNING | Line exceeds 80 characters; contains 126 characters
 63 | WARNING | Line exceeds 80 characters; contains 136 characters
 64 | WARNING | Line exceeds 80 characters; contains 125 characters
 65 | WARNING | Line exceeds 80 characters; contains 132 characters
 66 | WARNING | Line exceeds 80 characters; contains 137 characters
 68 | WARNING | Line exceeds 80 characters; contains 125 characters
 70 | WARNING | Line exceeds 80 characters; contains 136 characters
 71 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...modules/pareview_temp/test_candidate/views/autoarch_filter_multiple.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  8 | ERROR | Class name must begin with a capital letter
  8 | ERROR | Class name must use UpperCamel naming without underscores
  9 | ERROR | Method name "autoarch_filter_multiple::value_form" is not in
    |       | lowerCamel format, it must not contain underscores
  9 | ERROR | Missing function doc comment
 23 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

I'll do a manual review shortly.

I checked at your code, looks not bad, here some comments:

drupal_set_message(t("Your archivation settings for the selected node (nodeID: %nid) could NOT been updated, because the node's type is not selected at <a href='/admin/settings/autoarch'>Autoarch settings page</a>!", array('%nid' => $nid)), 'error');

You should use the l() function for adding links, also don't submit any html to the t() function!

if (!empty($nodes) AND !empty($state))
Use "&&" instead of "AND"

The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could for example start by evaluating my own project:
http://drupal.org/node/1302786

Yes, thanx for the quick reply luxpaparazzi!

What I agree with you in are the followings:

• shortening the lines of README.txt -> DONE
• changing the 'AND's to && of course -> DONE

What I do NOT agree or accept are the followings:

• First one:

You should use the l() function for adding links, also don't submit any html to the t() function!

There's in the drupal documentation how to use the t() function: http://api.drupal.org/api/drupal/includes%21common.inc/function/t/6
It says:

Here is an example of t() used correctly:

  $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';

So after all, the html tags ARE allowed in t() function. What I can do or maybe you meant like that, I can change the href's value to a string like this:

drupal_set_message(
t("Your archivation settings for the selected node (nodeID: %nid) could NOT been updated, because the node's type is not selected at <a href='@autoarch-settings-page'>Autoarch settings page</a>!",
  array('%nid' => $nid),
  array('@autoarch-settings-page' => url('/admin/settings/autoarch'))),
 'error');

Is that what you meant or is it OK for you then?

• The other thing I won't accept is the error given from Drupal Code Sniffer

I know it's not up to you or anyone, or anyone's fault, but I just copied the synthax from Domain Access' Domain Views moduls filter. Just to let you know what I mean this one is from domain_views_handler_filter_domain_access_gid.inc:

<?php
/**
 * @file
 * Interface between domain_views.module and views.module.
 *
 * @ingroup domain_views
 */

/**
 * Filter for group ids stored in the database, currently active domain and any domain
 */
class domain_views_handler_filter_domain_access_gid extends views_handler_filter_in_operator  {
  function get_value_options() {
    if (!isset($this->value_options)) {
      $this->value_title = t('Domain');
      $domain_options = array('-1' => t('Any domain -- use with "Is not one of"'), '***CURRENT_DOMAIN***' => t('Current Domain'));
      $domains = domain_domains();
      foreach ($domains as $domain) {
        $domain_options[$domain['domain_id']] = check_plain($domain['sitename']);
        }
      $this->value_options = $domain_options;
    }
  }
}

so the synthax is the same, I can nothing do about it, and honestly I don't want eather :)
If it's ok for a huge module like domain access, then it's ok for me too...

Please let me know if after this I'm wrong!
Thanks again!

I would at least check the following erros:

<blockquote>  8 | ERROR | Class name must begin with a capital letter
  8 | ERROR | Class name must use UpperCamel naming without underscores
  9 | ERROR | Method name "autoarch_filter_multiple::value_form" is not in
    |       | lowerCamel format, it must not contain underscores</blockquote>

It's better checking and comparing with core modules, as there is no guarantee that contrib modules got every coding standard correct.

So after all, the html tags ARE allowed in t() function.

Well ok maybe it is accepted, but won't make live easier for people doing translations, that's why I personaly don't like it, but it's more a personal opinion.

---

The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could for example start by evaluating my own project:
http://drupal.org/node/1302786

Thanks for the quick reply!

I've made some changes in the code, now only two ERRORs left after Code Sniffer:

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
67 | WARNING | Line exceeds 80 characters; contains 109 characters
--------------------------------------------------------------------------------

FILE: ...l/modules/pareview_temp/test_candidate/views/AutoarchFilterMultiple.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
13 | ERROR | Method name "AutoarchFilterMultiple::value_form" is not in
| | lowerCamel format, it must not contain underscores
--------------------------------------------------------------------------------

The first one is an address to a documentation in the README file, and the second is views own form function, what I can do with that, is makeing a function substitution. The question for that 'does it worth' ? I can make the changes but i don't feel that really necessary.

I've also made changes in the t() function mentioned above.

I attached some screenshots for introducing the features of this module.

review bonus #1 added

The first one is an address to a documentation in the README file, and the second is views own form function, what I can do with that, is makeing a function substitution. The question for that 'does it worth' ? I can make the changes but i don't feel that really necessary.

I suppose it's not worth it, and would make comparing with view module harder, it's not the goal of naming conventions. I suppose in the views module it is something which rested in there since several years. And changing would bring compatibility issues with other modules.

I suppose it's not worth it, and would make comparing with view module harder, it's not the goal of naming conventions. I suppose in the views module it is something which rested in there since several years. And changing would bring compatibility issues with other modules.

That's right :)

added a review link

review added

I find your reviews very short, you may have a look at this post:

Posted by patrickd on April 24, 2012 at 10:05pm

Thanks for your participation, but I'm afraid the comments you did are not very detailed and IMHO not really "reviews".
A good deep review can take up to an hour, this is not about just pointing out 2 things and set the issues to needs work.
Sorry, removing tag.
http://drupal.org/node/1540804#comment-5912808

thanx for the comment. We will keep that in mind,however we don't just push the code into a code sniffer, when checking a module we are interested. We don't check a module because of should but cuz we find that interesting. Anyway personally i just hope we'll be able to share our module (and future modules) to the community.

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

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

  FILE: ...drupal-7/sites/all/modules/pareview_temp/test_candidate/autoarch.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   320 | ERROR | Expected 1 space between comma and argument "$date"; 0 found
  --------------------------------------------------------------------------------
  

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. autoarch_perm(): do not run the permission machine name string through t(), as it is an identifier.
2. "Implements of hook_perm()." should be "Implements hook_perm().", see http://drupal.org/node/1354#hookimpl
3. autoarch_admin_settings(): you can remove that function and use drupal_get_form as page callback directly in hook_menu().
4. autoarch_admin_form(): you should use system_settings_form() here, then you don't need your own submit callback.
5. autoarch_admin_form(): in Drupal 6 it is necessary to sanitize #options on checkboxes in the form API to avoid XSS exploits. See http://drupal.org/node/28984
6. "WHERE nid=%d": there should be a space before and after the "=".
7. autoarch_form_alter(): you should check for the form id as early as possible to avoid a lot of unnecessary function calls.
8. autoarch_form_alter(): "$form['nid']['#value']": that is not available on every form in the system and will throw PHP notices. Please enable PHP notices in your php.ini in your development environment.
9. autoarch_get_nodetypes(): why does the default array contain the empty string? Also elsewhere.
10. autoarch_schema(): why do you save the data as string? Why can't you save it as unix timestamp as done in the rest of drupal? This would also have the benefit that you don't have to load all records in autoarch_cron(). You could specifically query for the relevant records which would heavily optimize the memory footprint on sites with a large amount of entries.

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

Thx klausi for the constructive comment! It was useful, you picked up things I didn't know or never used before :)

autoarch_perm(): do not run the permission machine name string through t(), as it is an identifier.

yeah, it's ok, removed the the string from t()...DONE

"Implements of hook_perm()." should be "Implements hook_perm().", see http://drupal.org/node/1354#hookimpl

Sure, i missed just that one...DONE

autoarch_admin_settings(): you can remove that function and use drupal_get_form as page callback directly in hook_menu().

Ok, I removed the function and changed the call to direct call...DONE

autoarch_admin_form(): you should use system_settings_form() here, then you don't need your own submit callback.

Ok, removed the submit button and added the system_settings_form()...DONE

autoarch_admin_form(): in Drupal 6 it is necessary to sanitize #options on checkboxes in the form API to avoid XSS exploits. See http://drupal.org/node/28984

Ok, sanitized the checkbox #options ( array_map('check_plain', node_get_types('names')) ) and also added a security check into submit handler...DONE

"WHERE nid=%d": there should be a space before and after the "=".

Yes, again I missed that...DONE

autoarch_form_alter(): you should check for the form id as early as possible to avoid a lot of unnecessary function calls.

Ok, I've put a form_id check at the beginning of function, if that's what you mean...DONE

autoarch_form_alter(): "$form['nid']['#value']": that is not available on every form in the system and will throw PHP notices. Please enable PHP notices in your php.ini in your development environment.

Ok, I'm logging everything, just forgot about notices. I've put a check for that value into form_alter...DONE

autoarch_get_nodetypes(): why does the default array contain the empty string? Also elsewhere.

No comment, just DONE

autoarch_schema(): why do you save the data as string? Why can't you save it as unix timestamp as done in the rest of drupal? This would also have the benefit that you don't have to load all records in autoarch_cron(). You could specifically query for the relevant records which would heavily optimize the memory footprint on sites with a large amount of entries.

Ok, now the thing is, when I started to implement things I couldn't think for future advance, so at the beginning I thought I would use as string. Then I changed my mind and turned to unix timestamp, because of it's better usability. Then I also forgot to change it at schema level and in cron...sorry about that, anyway...DONE

security tag is for statistical purposes, please leave it

Sorry for the delay. Make sure to review more project applications and get a new review bonus to get this finished faster.

manual review:

1. autoarch_perm(): permissions should be all lower case.
2. autoarch_access(): wrong parameters, see http://api.drupal.org/api/drupal/developer!hooks!node.php/function/hook_...
3. autoarch_access(): why do you even need that function, you are not defining your own node type?
4. autoarch_admin_form_submit(): why do you unset $form_state['storage'] ? Anyway, you can remove that function now, as you are using system_settings_form() now?
5. autoarch_form_alter(): no need to check if the form_id is empty, Drupal will always pass a valid id here.
6. autoarch_form_alter(): why do you need to access the raw #post values and cannot use $form_state['values'] ?
7. autoarch.js: you should use Drupal.behaviors, see http://drupal.org/node/304258
8. AutoarchFilterMultiple::query(): no need to use check_plain() here, as you are not printing anything to the user?

Othwerwise looks almost ready to me.

Hi klausi!

Replying to your comment:
1:Ok, changed 'A' to 'a'.
2 & 3: copied from here, because this way worked for me too. Anyway, removed the function, we'll see if that'll also work.
4: I've read somewhere about freeing up memory in drupal based systems, that's why I put unset in, but if not necessary than i can cut that out. No problem. However the whole function is necessary, because there're two important variable_set()'s in there. The system settings_form($form) does nothing else than adding the default buttons to the form, so that's why the submit call needed to handle the variables.
5: it was mentioned by you in comment #17 at point 7. No further comment.
6: because of the difference between (creating/posting a new node) & (editing an already created node). Because a new node doesn't have a nid yet, but a created does.
7: there's no such thing in a module like Admin Menu's admin_menu.js! There's no $(document).ready() function in my module's js either, to replace with Drupal.behaviors! It doesn't need to be attached to Drupal.attachBehaviors(), it should run only once the page loads. It's specific, not reusable, that's why it's not necessary to put in Drupal.behaviors() sandwich.
8: '%s' is gonna be $exposed_value, so it must be "check_plain"-ed, or the whole query dies.

Replying for the comment above:
Any further problem-idea-submission-needs-etc can go to the module's issue list, there's no more real problem with it, just please accept/allow the module! First it goes to dev version, not as stable release, but yet we can't reach that stage. I'm personally getting listless sharing to the community by this procedure :(

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review

1. autoarch_form_alter(): I said that you should compare the form id as early as possible with $autoarch_form_types which you are doing now. But the empty() check at the very beginning is still useless.
2. autoarch_admin_form_submit(): no, system_settings_form() will also add its own custom submit handler and save variables for you. So currently variables are saved saved twice with different names I think, which is very confusing.
3. AutoarchFilterMultiple::query(): why should the query die if $exposed_value is not run through check_plain()? Please read http://drupal.org/node/28984 again: sanitization must only be done on output, everything that is not printed to the user must be left alone. What am I missing here, how will Views print $exposed_value to the user?
4. "'access arguments' => array("Administer autoarch settings"),": still using the upper case permission.
5. autoarch_get_date_granularity(): the break statements are unreachable code, remove them.

Although you should definitively fix those issues they are no blockers. The permission problem is not a security issue as the permission will just be ignored currently and your module's administration form is only usable by user 1. So I guess this is RTBC.

Your application is only RTBC not fixed yet, a git admin must do a final review and will then grant you permission to promote projects to full ones.

Modifications done...

what about now (I'm sorry they're 20 other waiting for this too..)

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch:

git branch -D master
git push origin :master

The @file block in your readme is not necessary (nobody does that)

Thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.