BUEditor Plus

welcome

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• bueditor_plus.admin.inc in bueditor_plus.info: It's only necessary to declare files[] if they declare a class or interface.
• ./bueditor_plus.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function bueditor_allowed_fields() {
  function bueditor_profile_load($pid) {
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./README.txt:              ASCII English text, with CRLF line terminators
  README.txt
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  bueditor_plus.info:1:; $Id$ 
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxhollyit1289922git.

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. Go and review some other project applications, so we can get back to yours sooner.

Code review results of bueditor_plus.module file:

      if (isset($settings['BUEPlus'])) {
        drupal_add_js($settings, 'setting');
        _bueditor_plus_add_js();
      }

• You use drupal_add_js() to add a javascript file. In Drupal 7 the preferred method in a form context or renderable array context is to use #attached.
• With _bueditor_plus_add_js() you call drupal_add_js() once. Is this to prevent a js file from being added multiple times? No need, drupal_add_js takes care of that.

/**
 * @file
 */

• Add some file documentation
• Document the hooks according to the conventions. Including empty lines.

Changing state accordingly.

The HTML is not sent. Otherwise your drupal_add_js() would not work. As for as I know Drupal sends the full HTML only after it finished rendering.

I see no changes in the other documentation. These may seem silly details, but developers care about code style. It makes code better transferable between developers and with that it supports collaboration.

• Implements hook_* instead of Implements of hook_*
• Insert empty lines in the function documentation

Well, the purpose of this review is not to make the best possible module, but to make sure it passes the criteria and to help you learn the Drupal API and the Drupal standards. There is always room for improvement.

If you take care of the documentation I will continue with the review of the other files.

Code review of bueditor_plus.admin.inc:

/**
 * Main administration page. Overrides the default BUEditor admin page
 */

The comment says the BUEditor admin page is overridden, but you ADD a definition to admin/config/content/bueditor in the hook_menu() implementation. Use hook_menu_alter() instead.

  $rows[] = array(
    '',
    l(t('Add new profile'), $path . '/new'),
  );

Use the Local Action UI pattern for the Add new profile link.

  return confirm_form($form, t('Delete profile :profile', array(':profile' => $profile->name)), 'admin/config/content/bueditor', t('This action cannot be undone.'));

• ':profile' is not an allowed replacement pattern in the t() function. Use %profile instead.
• t('This action cannot be undone.') is the default description. It can be omitted.

/**
 * Delivers the confirmation to delete $profile
 *

Tip: prefix this comment with 'Menu callback: '. This makes searching and recognizing callbacks easier.

  $form['name'] = array(
    '#type' => 'textfield',
    '#title' => t('Profile Name'),
    '#description' => t('Give your profile a unique name to easily identify it.'),
    '#required' => TRUE,
    '#default_value' => $profile ? $profile->name : '',
  );

Instead of checking the presence of $profile in every form element (e.g. $profile ? $profile->name : '') Check once if $profile is NULL and define a default profile.

    db_query("UPDATE {bueditor_plus_profiles} SET global=0");

db_query() is not meant to be used for update queries. Use db_update() instead.

  if (isset($form['#profile'])) {
    db_update('bueditor_plus_profiles')
    ...
    drupal_set_message(t('Profile has been updated'));
  } 
  else {
    db_insert('bueditor_plus_profiles')
    ...
    drupal_set_message(t('New profile created'));
  }

Use db_merge() to combine db_update() and db_insert().

I found some security faults:

• Output of profile names are not sanitized.
• Output of text formats are not sanitized.
• Always sanitize the output! Use check_plain() to display as plain text.

If you want to maintain a CHANGELOG.txt make sure you keep it up to date. Personally I can't. So I rely on the issue queue and git log.

Jamie, thanks for your quick reply's and prompt corrections. To me the module is ready, and complies to standards. I hope you feel more comfortable with the Drupal API and are more aware of Drupal's code style and security.

May your hands always be busy, May your code always be used ;)

Review of the 7.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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• "php = 5.2.x": drupal 7 core already requires PHP 5.2, so remove that line.
• bueditor_plus_install(): juggling with module weights is ugly and so Drupal 6 style. I assume that you want to make sure to run your hooks before/after bueditor, in that case you can make use of hook_module_implements_alter().
• bueditor_plus_update_7001(): please break the array up into several lines, we are used to it like in hook_schema().
• bueditor_plus_menu(): "access argument" is missing on all your menu items.
• "drupal_set_message(t('The profile :profile has been deleted', array(':profile' => $profile->name)));": The ":" placeholder does not exist, I think you want "@" or "%" here.
• "'#title' => 'Global profile',": make sure to run all user facing text through t() for translation.

Sorry, I overlooked the old common array in hook_menu().

Green lights from me.

Hey @Jamie Holly, you now have full Git access. Happy hunting.

Thanks to all the reviewers!