Noty Messages

[EDIT] Oops, It seems I cloned default branch.

There are some issues you can work:
http://ventral.org/pareview/httpgitdrupalorgsandboxmrmaksimize1449608git

  // Check for lib path existence.
  if (!is_file(libraries_get_path('noty') . '/js/jquery.noty.js')) {
    return FALSE;
  }

I think it's a not good idea to check libraries for every message. Use hook_system_requirements() instead.

dependencies[] = ctools
Please clarify me by which way it used in your module.

As stated in documentation hook_system_requirements() should be placed in module_name.install file.

Note that this hook, like all others dealing with installation and updates, must reside in a module_name.install file, or it will not properly abort the installation of the module if a critical requirement is missing.

function noty_messages_enable() {
  if (!is_file(libraries_get_path('noty') . '/js/jquery.noty.js')) {
    drupal_set_message(t('Please install noty library'), $type = 'warning');
  }
}

It looks as $type variable is not used.

'#title' => t('@type messages', array('@type' => ucfirst($message_type))),

As ucfirst() is not multibyte-compatible Drupal has custom implementations of the function.
drupal_ucfirst()

@Chi, thanks for your help, but would you mind to edit your last comments instead of posting a new one for every issue you find? (Just for readability) ;-)

This looks like a really useful module - I would love to start using this in place of something like Better Messages as this certainly seems to provide more visual configuration options and a "more current" UI. Very neat!

Changing status as these things seem to be fixed.

Hi

1) noty_messages.admin.inc
You can use $form['#attached']['js'] instead of drupal_add_js() in your forms (like noty_messages_admin_settings) so others can easily manipulate it.

2) noty_messages.admin.inc
t() should not contain tags.

http://drupal.org/node/322731

$message = t("If you don't want to receive such e-mails, you can change your settings at !url.", array('!url' => l(t('My account'), "user/$account->uid")));
$title = t("@name's blog", array('@name' => $account->name));
$message = t('%name-from sent %name-to an e-mail.', array('%name-from' => $user->name, '%name-to' => $account->name));

3) noty_messages.admin.inc
Do you always need the variable $disabled in noty_messages_admin_settings ? If the js is not present you will not display some of these fields anyway ? And in 'noty_messages_generate_settings_form' it seems $disabled is always FALSE ?

I will leave it to needs review as I didn't spot anything critical so you can get other reviews. But we do really need more hands in the application queue and it is highly recommend to get a review bonus so we will come back to your application sooner.

Check : http://drupal.org/node/1011698

See also #1410826: [META] Review bonus

Thanks,

1. Why did you defined these constants? I found only three used in the rest of the code.

   // Passed to JS.
   define('NOTY_LAYOUT', 'topRight');
   define('NOTY_TEXT_ALIGN', 'left');
   define('NOTY_SPEED', 500);
   define('NOTY_TIMEOUT', 5000);
   define('NOTY_CLOSABLE', TRUE);
   define('NOTY_CLICK_CLOSE', TRUE);
   define('NOTY_MODAL', FALSE);
   define('NOTY_TYPE', 'success');
   define('NOTY_THEME', 'noty_theme_default');
   
   // Config.
   define('NOTY_ADMIN_PATH_DISABLE', FALSE);
   define('NOTY_IS_NOTY', FALSE);
   define('NOTY_USE_GLOBAL', TRUE);

2. Also, I found many unused variables:
   

   -  noty_messages_messages_setup($admin = TRUE);
   +  noty_messages_messages_setup(TRUE);

   -   $disabled = TRUE;
       return $form;

   function noty_messages_generate_settings_form(&$form, $message_type, $defaults, $disabled = FALSE) {
     $settings_key = $message_type;
   -  $disabled = FALSE;

   -  $custom_css = noty_messages_get_custom_css($themelayer = TRUE);
   +  $custom_css = noty_messages_get_custom_css(TRUE);

   -  $custom_css = noty_messages_get_custom_css($themelayer = TRUE);
   -  if (!empty($custom_css)) {
   +  if (!noty_messages_get_custom_css(TRUE)) {
       drupal_add_css($custom_css);
     }

3. You are using empty function everywhere but actually it isn't required for bool values.

   - if (empty($admin)) 
   + if (!$admin)
   

4. As you said promise.js makes it possible to use Noty Library with older jQuery versions but the module still has a dependency on jquery_update module. Are there any reasons for this?

   dependencies[] = jquery_update
   

5. Do not use newlines and multiple spaces inside t() it makes the string untranslatable. Also seems that's incorrect path "admin/settings/noty"

     if ($path == 'admin/settings/noty') {
       return t('Set your options and decide whether you want the noty styled messages
         to appear on admin pages or not.');
     }

I'm assuming you're talking about the admin variable in noty_messages_setup.

I mean noty_messages_admin_settings(). I cannot find $admin variable within the function. Here is the code.

function noty_messages_admin_settings() {
  // Per message configs
  // Clear out the form first.
  noty_messages_messages_setup($admin = TRUE);

There is another unused variable $themelayer inside noty_messages_messages_setup():
$custom_css = noty_messages_get_custom_css($themelayer = TRUE);

Always check $is_noty in noty_messages_type() it seems that the variable will never change. $message_types in noty_messages_settings_reset() seems as unused.

empty() usage if just something I'm used to using to do what I believe are safe checks with booleans. I had bad experience with !$variable checks before.

There are no any reasons to use empty() if you are sure that a variable is set and it can be evaluated to TRUE or FALSE.

Here some other examples:

-  if (empty($theme)) {
-    // We're looking for core noty css additions.
-    $file = 'noty.css';
-  }
-  else {
-    $file = $theme . '.css';
-  }
+  $file = $theme ? $theme . '.css' : $theme . '.css';

-      '#default_value' => empty($defaults['use_global'][$message_type]) ? FALSE : TRUE,
+      '#default_value' => !empty($defaults['use_global'][$message_type]) ,

-  if (strstr($message, '<pre>') || strstr($message, '<textarea') ||
-    strstr($message, 'krumo')) {
-    // Devel message found.
-    return TRUE;
-  }
-  else {
-    return FALSE;
-  }
+  return preg_match('/<pre|<textarea|krumo/', $message);

-  $output = '';
-  $output .= "<div class=\"messages $type\">\n";
+  $output = "<div class=\"messages $type\">\n";

-  if ($themelayer == TRUE) {
+  if ($themelayer) {

There is a lot of such similar micro issues in your code. It isn't a performance issue or a code standards issue it just makes your code clear and unambiguous.

I would not recommend changing from strstr() to preg_match() for performance reasons, though switching to strpos() would be advisable (see the note on http://www.php.net/manual/en/function.strstr.php).

$message = 'Lorem ipsum dolor sit amet, consectetuer adipiscing elit, sed diam nonummy nibh euismod tincidunt ut laoreet dolore magna aliquam erat volutpat.';
$start = microtime(1);
$result =  preg_match('/<pre|<textarea|krumo/', $message);
$end = microtime(1);
echo round(1000*($end - $start), 3) . ' ms';

I got about 0.03 ms. It's too little to take into account (especially in Drupal).

But actually it doesn't matter what are you using preg_match or strpos. I just pointed on code style.

-  if ($bool_value) {
-    return TRUE;
-  }
-  else {
-    return FALSE;
-  }
+  return $bool_value;

What about $is_noty variable in this function? It seems that the variable will never change.
Also did not found by which way $messages parameter is used.

function noty_messages_type($type, $messages) {
  // Assume TRUE.
  $is_noty = TRUE;
  // Check type visibility.
  $active_types = variable_get('noty_messages_is_noty', _noty_messages_get_default(array($type), NOTY_IS_NOTY));
  if (empty($active_types[$type])) {
    return FALSE;
  }
  // Should the messages be shown on admin paths?
  if (path_is_admin(current_path())) {
    $admin_path_settings = variable_get('noty_messages_admin_path_disable', _noty_messages_get_default(array($type), NOTY_ADMIN_PATH_DISABLE));
    if (!empty($admin_path_settings[$type])) {
      return FALSE;
    }
  }
  // If no other conditions have return FALSE by now, show type.
  return $is_noty;
}

Sometimes it is good practice to pre-initialize your arrays when applicable so you don't accidentally populate data to an existing array with the same name defined elsewhere in the code. I didn't find any information about it in Drupal code standards but core modules don't use this practice. See node_block_info() for example.

 function noty_messages_admin_settings() {
   // Per message configs
   // Clear out the form first.
   noty_messages_messages_setup(TRUE);
-  $form = array();

Take look at the noty_messages_status_messages() seems as $output was initialized twice. Also check $noty_out that seems to be unused.

 function noty_messages_uninstall() {
-  module_load_include('module', 'noty_messages', 'noty_messages');
+  module_load_include('module', 'noty_messages');
-   $config_keys = _noty_messages_get_config();
-   foreach ($config_keys as $key) {
+   foreach (noty_messages_get_config() as $key) {
     variable_del('noty_messages_' . $key);
   }
 }

There is discussion about deletion of variables prefixed with your module's name.
http://drupal.stackexchange.com/a/2476/432

Yup, about variables you are right. 'noty_messages_' . $key seems to be safe.

Shouldn't you use http://git.drupal.org/sandbox/MrMaksimize/1449608.git as your GIT-URL?

Respect for your inline-documentation, I should probably hire your for documenting my own code ;)

I did not see any major problems, but I'll definetly correct the following:

I'll put an empty line between function calls

}
/**

found before:
- before noty_messages_build_messages()
- noty_messages_generate_settings_form()

HTML should not be put into t() function:

      $requirements['noty_messages_js'] = array(
        'value' => $t('Missing!'),
        'severity' => REQUIREMENT_ERROR,
        'description' => $t('Noty library missing. Download the Noty library from <a href="@url">http://needim.github.com/noty/</a> and place in in to sites/all/libraries/noty', array('@url' => 'http://needim.github.com/noty/')),
      );

I let the status "needs review" for other people having a look at it.

btw, it's kind of allowed to put a-tags this way into t() functions (just have a look at core hook_help implementations [I don't like that either])

Formatting

I suppose that submit handler will not be executed if form validation failed.

 function noty_messages_admin_settings_submit($form, &$form_state) {
-  if (!form_get_errors()) {

It seems you reinvent the wheel. There is a simple drupal function which can help you add default buttons and reset module settings.
system_settings_form

- 'reset' => array(
-   '#type' => 'submit',
-  '#value' => t('Reset all settings'),
-  '#submit' => array('noty_messages_settings_reset'),
- ),

- function noty_messages_settings_reset($form, &$form_state) {
-   $defaults = _noty_messages_get_config();
-   foreach ($defaults as $key => $value) {
-    variable_del('noty_messages_' . $value);
-  }
- }

I am wondering is there a real need to have "global messages settings"? I think it doesn't bring any UX profits for such simple module as noty_messages.
The same question about custom css functionality. There is a lot of ways to add custom css. Why do you want to make your own method for this?
Removal of these features make your module more maintainable.
Of course it is my subjective opinion :-)

Oops, in D7 the reset button has been removed, but it is not documented yet.
#518750: Rethink the system settings form

1. now as the d.o/project/noty namespace already exists, why don't merging the code in there to prevent duplicating namespaces and confusion?
2. php = 5.1 drupal7 requires 5.2, therefore senseless here
3. Your readme contains good information but looks very unstructured, have a look at typical readme styles
4. use /**/ comment style instead of // when your commenting in global space
5. begin all comments uppercase and end them with fullstop / ? / !
6. use the full module name when prefixing variables
7. wrap translatable test in t(), also in hook_permission (but not in hook_menu or watchdog())
8. IMHO your hook_help implementation is not very helpful (at least not worth spending runtime with), maybe you either remove it or provide more help
9. 'access callback' => 'user_access', this is a default value, you don't have to set it
10. 'original_status_messages' you probably should also prefix that theme function with your full module name

11. function noty_messages_uninstall() {
      module_load_include('module', 'noty_messages');

    (not tested it) but are you sure this is necessary ?

12. is it really necessary to reinvent the configuration system ? ^^ but okay, if you want it that way, why not
13. array('type' => 'setting', 'cache' => FALSE)cache key only works for added files, neither for settings nor for inline js
14. I'm also not sure whether noty_messages_get_custom_css() is really needed, maybe I missed it but why exactly are you doing it that way? wouldn't just overriding it by the theme's css not work?
15. add your configuration page with "configure = " (https://drupal.org/node/542202) to your .info file
16. It's a little hard to find the actual download page of noty from the current link, maybe you use http://needim.github.com/noty/#installation instead
17. make sure to check that speed and timeout settings are positive integers (eg. by element validation and element_validate_integer_positive)

but this looks all pretty good to me, we'll get this done soon ;)

I've removed the "review bonus" tag, you can add it again after 3 more reviews, remember to pick old issues first
thanks

regards

manual review:

1. noty_messages_admin_settings(): do not check for the library here again, this should be done only once in hook_requirements().
2. noty_messages_admin_settings_submit(): why do you check form_get_errors() here? You never use form_set_error(), so there will never be errors?
3. theme_noty_messages_original_status_messages(): don't create list markup yourself, use theme('item_list', ...) instead. Hm, I see that drupal core does not use it, so this is just a suggestion.

But that are just minor issues, so ...

Thanks for your contribution, MrMaksimize! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.