[D7] AJAX Update

Hi @gwprod. Your clone command should be something like:
git clone http://git.drupal.org/sandbox/gwprod/2298383.git
- users will not be able to clone your personal repo. Also, it looks you haven't set your default branch to 7.x-1.x. You need to do this, as well.

Hey, @gwprod.

Minor stuff returned when running code sniffer relating to ajax_update.js:

--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 28 | ERROR | Whitespace found at end of line
 32 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

I installed your module, and functionally, it works fine, but it would be helpful if you could add some specific use cases to the README, i.e., when and why this module should be used.

I would change the nested conditional you have in _ajax_update_html_id_forms_process in your .modules file - you could accomplish this with

if (isset($form['#attributes']['class']) && !in_array('ajax_html_id_exclude', $form['#attributes']['class'])) {
      // Add the ajax_html_id_exclude class to the form.
      $form['#attributes']['class'][] = 'ajax_html_id_exclude';
  }
  else {
    $form['#attributes']['class'] = array('ajax_html_id_exclude');
  }

I'd also add a link to the config page from the modules page, in your .info file, like so:

configure = admin/config/ajax_update

Elsewise, everything looks good.

I've reviewed @gwprod's last three commits:
http://drupalcode.org/sandbox/gwprod/2298383.git/commit/ed7d1d3
http://drupalcode.org/sandbox/gwprod/2298383.git/commit/0db1dc5
http://drupalcode.org/sandbox/gwprod/2298383.git/commit/42606c1

The issues I referenced above have all been addressed and module passes functionality testing. Would change status to "Reviewed & tested by the community," but thus far I'm the only tester - should probably have another set of eyes on this first.

A couple minor things on first look. Some are more of an FYI (incase you weren't aware) and not really a release blocker:

• Have you considered changing your ajax_update_admin to use system_settings_form
• 'access callback' => 'user_access', is not necessary. That is the default callback and can be left blank.
• Same goes for 'type' => MENU_NORMAL_ITEM,
• In ajax_update.module: $settings = variable_get('ajax_update');, you should probably have the default value as the second argument to variable_get (which also makes the next conditional unnecessary).
• In ajax_update.module, the else on line 134 is not necessary, since it will only be reached if the first condition is not met anyway.
• In the same function, you don't need to check if the children array is empty. You can just do foreach (element_children($element) as $...).
• ajax_update.js has improper coding standards (2 spaces not tabs, bracket on same line as conditional, etc).

Again, nothing too serious, but worth pointing out.

Looks good. Passed automatic pareview.sh, and conforms to the project application checklist. Switching to "Review & tested by the community". But, there are a some minor items/questions:

1) In the README.txt, in the CONFIGURATION section I think the first bullet should read something like: "Configure AJAX Update in the administration menu under Configuration -> Ajax Update -> Configuration."
2) On the configuration page there is a "Submit" button and a "Save Configuration" button. I think the "Submit" button should go away.
3) I think you might want a radio button on the configuration page to disable the functionality. Currently the only way to disable it is to disable the module. But perhaps that's good enough.
4) You might want to clarify in the documentation that not all ajax_html_ids[] are removed, just those associated with the form elements. From the tests I ran there were still some that were posted. Those look liked they were associated with the admin toolbar.
5) Can you give an example in the documentation for when this module is most useful? For example, on the form I tested, the AJAX always returned the same form elements with the same IDs (just with different values set). When AJAX was subsequently posted again the same form_html_ids looked to be submitted again. So, what is an example where different ajax_form_ids are posted differently? Is it when some form elements disappear and new ones are added?

Assigning to myself for review.

Automated Review

Review of the 7.x-1.x branch (commit 381f1bd):

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

  
  FILE: /home/matt/PAR/pareview_temp/ajax_update.js
  --------------------------------------------------------------------------------
  FOUND 4 ERRORS AND 1 WARNING AFFECTING 4 LINES
  --------------------------------------------------------------------------------
    5 | ERROR   | [ ] There must be exactly one blank line after the file comment
    5 | ERROR   | [ ] Additional blank lines found at end of doc comment
    7 | ERROR   | [ ] Inline doc block comments are not allowed; use "/* Comment
      |         |     */" or "// Comment" instead
   32 | ERROR   | [x] Functions must not contain multiple empty lines in a row;
      |         |     found 2 empty lines
   38 | WARNING | [ ] There must be no blank line following an inline comment
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  
  Time: 201ms; Memory: 6.5Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

In ajax_update_admin_form(), the variable_get() needs a default value.

The fieldset in the setting form is a little overkill. Take a look at the simpler core setting forms for inspiration.

Single quoted strings are preferred per Drupal coding standards.

So, you are monkey patching Drupal.ajax.prototype.beforeSerialize? This needs to be commented. Also note your changes.

Your indentation in the JS should be two spaces per level per Drupal coding standards.

No way to specify form IDs via the UI? Could be handy.

Your nesting / menu placement is a little weird in ajax_update_menu(). I think you can just stick this in admin/config/development with a single menu item.

You do not need the variable_set() in ajax_update_form_alter. Just rely on the default.

Were you getting notices in _ajax_update_html_id_forms_process? The first isset is overkill. Just tack the class onto the array. You also don't need the array_unique line.

It looks like the |= isn't needed since you return after a TRUE. You can likely simplify this a bit.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

None of this is major, and this has been RTBC for a while now.

Thanks for your contribution, gwprod!

I updated your account so you can 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 stay 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.