Javascript Drupal Extension

Interesting module. Kudos on the excellent inline comments!

I have looked through the code and have some questions and comments you may want to look into:

project page

OK

Master Branch

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

README.txt

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

Inline documentation

Very well done!

Major coding standards / best practice issues

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at http://ventral.org/pareview/httpgitdrupalorgsandboxpdnagilum1828912git .

Manual review

• There seems to be some serious security issues with this module. What would prevent an attacker from calling db_query('DROP TABLE node') or the like through the javascript_drupal_extension_ajax_call() function? Perhaps consider a whitelist for allowed functions.
• How about making javascript_drupal_extension_ajax_callback_url a Drupal javascript setting instead of inline code?
• If the module is a utility for developers why not place it in the development section section of the administration instead of creating a new one just for this?
• admin/config/utilities/javascript-drupal-extension has access argument administer javascript_drupal_extension. However administer javascript_drupal_extension is not declared in hook_perm().
• The code for extracting variables from $_GET and $_POST is very similar and ripe for refactoring.
• When using system_settings_form() there is no need to set the variable values explicitly if your form entries have the same keys as your variable names.

I hope these comments can help improve this module and look forward to seing it develop.

Cheers!

project page
Please consider including a small discussion about when to use your module: It would be great to have a small guidance about the right approach some specific situations.

For example:
Need to retrieve and create nodes? -> use the services module
Need to send configuration data to the browser that is usable via JS? -> use standard drupal api to populate the global Drupal object
Need to call small dynamic functions? -> use your approach

code review
Your function javascript_drupal_extension_ajax_call will always return an empty string, because the success and failure ajax handlers are called after the surrounding function returns. The only and best ways to retrieve the result is to pass a callback function to the javascript_drupal_extension_ajax_call method.

Your php code handles the POST/GET variable 'output-is-json', but your JS is not sending this one?!

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Thanks for the explanation PDNagilum, now i see why it is working. Anyway i would suggest to change the behavior so that all functions do not return anything but require a callback as the last parameter. This is the standard behavior and good style in JS - of course this requires you to change the calling code, but it has advantages:
You dont block the browser while waiting for the http response and additionally JS-programmers better understand the call semantics.

I would also recommend to throw an exception when the response did not receive successfully (within your error handler) - returning an empty string can be confusing.

manual review:

1. javascript_drupal_extension_install(): No need to set variables upon installation as you can make use of default values with variable_get() anyway.
2. javascript_drupal_extension_help(): all user facing text must run through t() for translation.
3. javascript_drupal_extension_config(): I think it is easier if users input one function per line for the whitelist.
4. "foreach ($whitelist as $function) {": instead of looping over the list you could use drupal_map_assoc() and then check with isset() if the function exists.
5. javascript_drupal_extension_config_validate(): just set the callback_url form element to #required and you don't need to check that?
6. javascript_drupal_extension_config_validate(): You should check if the functions on the white list actually exist.
7. 'Allow parsing of _GET variables in codebehind': what is "codebehind"?

Although you should definitively fix those issues they are not critical application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

no objections for more than a week, so ...

Thanks for your contribution, PDNagilum!

I updated your account to let you 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 get 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.