maximpodorov [maximpodorov]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

Thanks for the explanation; I think it makes clear what is the purpose of the module, and the difference with existing projects.

Hi @maximpodorov, my apologies for the delay; this is all volunteer work. Please note that this could be a lengthy back and forth process reviewing your module (depending on a lot of things), but the goal for all of us is to get you a CVS account and make sure you are a responsible contributor. So, be patient. :)

Review, not in any specific order. Please note that some points are just examples that may apply in other places.

1. Having a basic README.txt is a good practice.
2. Your uninstall hook is good to delete variables, but you should be utilizing variable_del(). See http://zzolo.org/thoughts/tip-managing-variables-drupal-module
3. Files should have docblocks, as well as all functions. Overall, make sure you have read and are following coding standards. Note that these standards are for more than just PHP. http://drupal.org/coding-standards
4. Since you are using a variable amount of variables and depends on the number of block. It is suggested to store your own values in a separate table so you do not overuse the variabels table.
5. You should implement hook_help() for a minimal help section in the admin section.
6. You JS should not be inline. You should pass data to the JS side, then process it in the JS file.
7. You need to utilize Drupal.behaviors in your javascript. These get called on page load and should be called on any AJAX events. http://drupal.org/node/205296 (Just added better practical example)
8. Good use of attachBehaviors, but you should pass along the DOM element you are processing as context.
9. What is the goal of allowing arbitrary JS execution? This is a huge performance hit and a big security risk, and I don't see the use-case. I would suggest utilizing jQuery's event system to create an even point that someone can plug into.

Overall, you do a pretty good job of writing good Drupal code, but there are some key structures that need to change, specifically on the JS side.

Remember to change the status, when you upload new code.

Thanks for your patience. I am assigning this report to me because I will review the code tomorrow morning (which means between 18 hours from now).

1. /**
    * Implementation of hook_uninstall().
    */
   function ajaxblocks_uninstall() {
     // Get global variable array.
     global $conf;
     foreach (array_keys($conf) as $key) {
       // Find and remove variables that have the module prefix.
       if (strpos($key, 'ajaxblocks_') === 0) {
         variable_del($key);
       }
     }
   }
   

   The code could remove variables used by another module with a name starting with ajaxblocks_ (somebody could create a module named ajaxblocks_extension, or ajaxblocks_views, in example). Also, the code is scanning all the variables to get its own variables, when those variables should be well known once the code has been completed.

2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted, and how constants should be written.

3. /**
    * Additional submit handler for block settings form. Saves AJAX settings for the block.
    */
   function ajaxblocks_save_settings($form, &$form_state) {
     $param_name = 'ajaxblocks_' . $form_state['values']['module'] . '-' . $form_state['values']['delta'];
     $old_value = variable_get($param_name, '0');
     $new_value = $form_state['values'][$param_name];
     if ($old_value == $new_value) return;
     // Remove variable if its value is default (not using AJAX).
     if ($new_value == '0') variable_del($param_name);
     else variable_set($param_name, $new_value);
   }
   

   It's not clear to me the purpose of that code, which is not implemented in other modules. Why isn't the form using system_settings_form()?

4.     // Set 'q' parameter in order to make arg() work correctly.
       $_GET['q'] = $_GET['path'];
       // Block content goes through theme functions.
       init_theme();
   

   It's not clear to me why that is required to make arg() work, and why the code is not restoring the original value of $_GET['q']; some other modules could need to use arg(), and they would get a value that doesn't match the path of the page currently viewed.

5. /**
    * Implements hook_preprocess().
    */
   function ajaxblocks_preprocess(&$vars, $type) {
   }
   

   It would be better to implement hook_preprocess_page(), and hook_preprocess_block().

the module sets $_GET['q'] intentionally, in order to make any module or theme think the user is visiting the page for which we have to return the blocks. It has to set $_GET['q'], since arg() and drupal_is_front_page() functions use only this variable, and calling these two functions is a usual way in Drupal to decide what to show on the page.

As the module is using that trick for its own menu callback, I think it is more acceptable.

Thank you for your contribution! I am going to update your account.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.