[D7] aed - Attach External Databases

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

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. There is screenshot image in module folder, you should remove that.
2. You should use t() for all markup text.
3. Use drupal style tables with theme_table();
4. Can't install module on clean drupal 7.22.

I checked module and clicked Save configuration, page just refreshed and module was not installed.
Tried with new version and this time it was installed successfully. Dunno what was wrong the first time.

Hi,

In aed.install for deleting variables you should use something like this:

// Simple DB query to get the names of your variables.
  $results = db_select('variable', 'v')
    ->fields('v', array('name'))
    ->condition('name', 'aed_%', 'LIKE')
    ->execute();
  // Loop through and delete each of your variables.
  foreach ($results as $result) {
    variable_del($result->name);
  }

This way, if you will add another global to your module, you will not have to modify install file.

Cheers.

Hey.

aed_menu()

• You can omit MENU_NORMAL_ITEM, since it's default.
• Nice move would be to move page implementation into a separate files, so its coded will be loaded only on direct call to the page.

aed_edit_form()

• 54:No need to check user_access() since it's already done by aed_menu() ('access arguments').
• 68:Might be a good move to convert to dynamic query.

aed_form()

• 299:No need to check user_access() since it's already done by aed_menu() ('access arguments').
• 337:Might be a good move to convert to dynamic query.
• 883:Might be a good move to convert to dynamic query.

Thank you.

Looks pretty good to me; it's a shame the approval has taken so long considering this module is actually quite simple.
I haven't really encountered the need for multiple DBs in any of the work I've done so far, but the less direct fussing with the settings.php that I have to do while developing, the better.

Any comments I can make are kind of minor quibbles or maybe even feature requests (#2054127: Check if DB Actually Can be Connected)!

On line 293 of aed.module.inc you should put brackets around the table name so that prefixing automatically happens if necessary. Since this isn't a production module maybe that can slide, but I think you should fix that at some point.

Update: Got fixed. #2054149: Curly Braces around table name in query string are missing

I'll look at this now in the Project applications sprint

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

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

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/aed.install
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   48 | WARNING | Unused global variable $databases.
  --------------------------------------------------------------------------------
  

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:

1. aed_uninstall(): no need to drop your own table, Drupal core will handle that for you automatically.
2. aed_edit_form(): do not document $form and $form_state, see https://drupal.org/node/1354#forms . Also elsewhere.
3. aed_edit_form(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075
4. "'#markup' => 'Databases defined in settings.php:',": all user facing text must run through t() for translation.
5. aed_form_submit(): do not call access functions in the submit handler. The menu is protected with that permission already, so nobody else can execute that submit callback.
6. .gitignore: looks like that is not needed anymore?
7. README.txt: please remove the license part, a LICENSE.txt file with the GPL will be added automatically for packaged downloads.
8. aed_form(): this is vulnerable to XSS exploits. If I enter a database with a server name as <script>alert('XSS');</script> then I get a nasty javascript popup on the overview page. You need to sanitize user provided text before printing. Make sure to read https://drupal.org/node/28984 again. This is not a security blocker because the high level "administer site configuration" permission is necessary to inject the malicious code, which is a site owning permission anyway. Should still be fixed anyway!

Not critical application blockers, so ...

Thanks for your contribution, kwfinken!

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.