[D7] Notify log

Hi,

Your git clone url is incorrect. You have shared the git maintainer clone URL. Kindly correct the same so we can review.

A brief read through your code looks good!

The Automated project review for your project revealed some issues:

• You need to set the default branch to 7.x-1.x.
• The code style sniffer found 8 errors and 2 warnings affecting 10 lines.
  • 2 errors are because you passed a variable or the result of a function to t(), which introduces an XSS security problem and confuses the Translation Template Extractor.
    To fix the error on line 84, you can simply remove the call to the t() function, because strings passed to watchdog() are translated when they're displayed, not when they're stored — see the documentation on the watchdog() function's $message parameter for more and dblog_overview() for more information.
    To fix the error on line 181, you can simply remove the call to the t() function, because constant names are the same in every language.
  • 6 errors are related to conforming to Drupal documentation standards (you're supposed to put a new line between @return and the return type in a docblock).
• The Drupal best practices sniffer found 3 warnings affecting 3 lines. They all appear to be related to using drupal_add_css() or drupal_add_js() inside hook_init(), which is discouraged for performance reasons, and because Drupal also outputs JavaScript, RSS, etc. which you cannot embed CSS/JS into. I think you can move the code into hook_page_build() instead.

Hope this helps :) Good luck!

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.

I am not very sure if you can include jquery.growl in the package. I can't say if the license is conflicting with GPLv2. Do you think it would be safer to not package it and use libraries?

#growls {
  z-index: 50000;
  position: fixed;
}

This and many other blocks is using CSS id. This would make it very difficult for themers to override the style. Is it possible to use classes here?

t('Duration display notices on the screen.<br />Ignoring this options if "Static" was checked.')

Instead of saying this, how about using Form States API to hide the text field.

In README.txt:

Contents:
=========
1. ABOUT
3. INSTALLATION
5. CREDITS

You should add a sequence.

Also, from the code, it is clear the module works only on *nix systems (maybe Mac, but not sure). But I can't find this in the README file. I think you should document this. In fact, I would even go as far as suggesting to add a hook_requirements() for this.

I have set it to Needs work for all this. If you think it doesn't warrant, feel free to change it back.

@kala4ek: I can't find the license for the plugin in the repository. This is why there was a confusion because the module can't be packaged with another license -- hence the libraries suggestion.

This is the extract from the license. I couldn't find the notice in the repository anywhere.

The above copyright notice and this permission notice shall be
included in all copies or substantial portions of the Software.

PAReview: 3rd party code

jquery.growl.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

I tried this module on current project and it's very helpful. I can easy know about all PHP notices, even for ajax requests.

Hi kala4ek,
Handle Review

• When you go to '/admin/config/development/notify-log' after install of module option 'Browser (Growl)' is disabled but chosen, it is not logical. Provided that the library 'jquery.growl' is not installed!
• If uninstall module you will have 3 notice see Selection_071.png. As result settings variables still exist after module deleted.

Hello kala4ek,

I used your module and it works fine, i hope that you will continue work on it and will implement many features.

Two recommendations from me:
1) Please change $static variable name in notify_log_js_settings function. It's confuse me, such variable names usually used for static variables.
2) Not use notify_log_access function, use user_access instead, because user_access already have static.

Looks like RTBC for me

Review of the 7.x-1.x branch (commit 8445cf6):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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:

1. project page is a bit short, please follow the instructions at https://www.drupal.org/node/997024
2. notify_log_watchdog(): exec(): Warning: When allowing user-supplied data to be passed to this function, use escapeshellarg() or escapeshellcmd() to ensure that users cannot trick the system into executing arbitrary commands. From http://php.net/manual/en/function.exec.php . An attacker would need the "administer site configuration" permission which is for trusted users only to insert a malicious user name for example, but this should still be fixed and is a security threat.
3. "'#title' => 'Growl settings',": all user facing text must run through t() for translation, make sure to check all your strings.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. Just a recommendation: Use hook_help for including your readme file into drupal UI: https://www.drupal.org/node/161085#hook_help
2. Just a recommendation: Add Requirements section to readme file. The requirements section (required) shall make it clear whether this project requires anything outside of Drupal core to work (modules, libraries, etc).
3. Just a recommendation: Why is it neccessary to use html code in your field descriptions?
   

       $form['notify_log_type']['#description'] .= t(
         '<br />"Browser" way will be available only after installation the !link library.',
         array('!link' => l(t('jQuery Growl'), $library['download url']))
       );
   

   . I suggest to take out new line html code from t function like:

       $form['notify_log_type']['#description'] .= '<br />' . t('"Browser" way will be available only after installation the !link library.',
         array('!link' => l(t('jQuery Growl'), $library['download url']))
       );

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.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

aha, good to see this functionality, just gone through the manual review first and found these things needs to be updated.

in notify_log.js
Drupal.settings.notifyLog = undefined; // Use single quote

2nd there re few minor things are still remaining, please remove them as well.

Review of the 7.x-1.x branch (commit 2954e63):

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /var/www/drupal-7-pareview/pareview_temp/notify_log.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
---------------------------------------------------------------------------
144 | WARNING | Line exceeds 80 characters; contains 102 characters
164 | WARNING | Only string literals should be passed to t() where
| | possible
281 | WARNING | Only string literals should be passed to t() where
| | possible
---------------------------------------------------------------------------

Time: 211ms; Memory: 10.25Mb

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.
Source: http://pareview.sh/ - PAReview.sh online service

manual review:

• notify_log_system_capability(): why do you suppress PHP warnings here with the "@" operator? Please add a comment.

Otherwise looks good to me.

Assigning to dman as he might have time to take a final look at this.

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

Thanks for your contribution, kala4ek!

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.

Just leave it at "fixed", it will close automatically after 2 weeks.