[D7] Slide Ad

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:
Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards).
Please see the attachement file.

This is quiet usefull module.

Some issues I found:

• In slidead.info remove line 9 ,,version = "7.x-1.0beta" ’’ it's will be added by the deploy-system. => #881720: Remove "version = VERSION" from .info
• It is unusual to set variables in the install ,,variable_set('slidead_content', t('Hello world! This is the welcome message for my site.')); ’’
  better set it in module line 71 && 100 $content = variable_get('slidead_content', t('Hello world! This is the welcome message for my site.'));
• The use of variable 'slidead_title' is inconsistent / undefined in line 105;
• in lines 42-44 the if-condition is unnecessary (variable_get('slidead_width', 300) != null)

I see no vulnerability issue.

Hth
Micha

Please don't assign application issues to yourself (only the current reviewer should do this).

There are still issues in codestyle, see here: http://ventral.org/pareview/httpgitdrupalorgsandboxsoncco1441362git
please fix them.

Hint: There are drupal-style-checker for various editors (vim, eclipse, text-wrangler etc. => http://drupal.org/project/drupalcs), you get a adhoc review while developing.

Micha

ok, pareviewsh is pleased.

I found an old point which I had overseen till now:
the handling of the text_format-field `slidead_content` is not properly implemented.

1. line 70 & 99: the pre-definition of `slidead_content` should be like this
   

    array(
     'format' => 'filtered_html',  // default input-filter
     'value' => t('text'),
   ); 

2. line 106: The slidead-content should be rendered by check_markup() see: http://api.drupal.org/api/drupal/modules%21filter%21filter.module/functi...
   

The way implemented now the input of the user (admin) wouldn't be checked and direct pasted in the page. In my understanding just mild security issue, because of the needed permission for administrating this `block`.

Micha

I miss the second point: Converting the user input to a formatted output (line: 111) via check_markup().
Using an input-filter moves the security-responsibility to the admin ;-)

Micha

Ok, all of my points are fixed! If I had something to say about, I would promote your module for getting full-project-status. :-)

Maybe a good time to give your project-presentation (http://drupal.org/sandbox/soncco/1441362) some love. E.g. put that linked screenshot on that page to illustrate the usefulness of slidead at first sight.

Oh wait: The last changeset brought new codestyle issues. (Indention and trailing whitespace... )

FILE: ../slide_ad/slidead.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
  72 | ERROR | Array indentation error, expected 4 spaces but found 6
  73 | ERROR | Array indentation error, expected 4 spaces but found 6
  74 | ERROR | Array indentation error, expected 4 spaces but found 6
  75 | ERROR | Array indentation error, expected 4 spaces but found 6
  76 | ERROR | Array indentation error, expected 4 spaces but found 6
  77 | ERROR | Array indentation error, expected 4 spaces but found 6
  78 | ERROR | Array closing indentation error, expected 2 spaces but found 4
 103 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

Pls fix thees issues too: http://ventral.org/pareview/httpgitdrupalorgsandboxsoncco1441362git

Micha

To directly add screenshots, you need to be a git vetted user

Alternative you can simply upload your images https://drupal.org/node/add/image and put them into your project page with image tags

Looks good. Congratulations, now I promote your module for getting full-project-status. :-)

Micha

You have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

1. There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
2. There is a git tag that has the same name as the branch 7.x-1.x. Make sure to remove this tag to avoid confusion.
3. "'access arguments' => array('access administration pages')," this permission is too generic, use your own.

Don't forget to add the review bonus tag and to add the review links to the issue summary.

Adding screenshot

Went through the code and found no obvious errors. Whatever that means, due to me being a pretty greenhorn. :)

Hi,

manually reviewed:

slidead.js - are you sure you need to call JQuery selectors without "context"

 var popup = $('#slidead-popup');
 ...
 $('#pop-close span').click(...);

To prevent processing of all document and dublicating attached eventListeneres each time the attachBehabiours is called you should use

 var popup = $('#slidead-popup', context);
 ...
 $('#pop-close span', context).click(...);

see what I mean?

Also automated review:

slidead.module:103:1: error - Whitespace found at end of line

Apart from this the code of the module looks ok and ready. Thanks for your work.

Don't forget to set the status to "needs review" if you want to get a review. See http://drupal.org/node/532400

manual review:

1. "'access arguments' => array('config slide ad'),": this does does not match the permission specified in slidead_permission()?
2. slidead_init(): if you need you Javascript or CSS on every single page request you should specify that in the info file. See http://drupal.org/node/756722
3. slidead_page_build(): I think you should inject your javascript setting here, not in hook_init(). Then you can remove hook_init() alltogether.
4. slidead_page_build(): this vulnerable to XSS exploits from the sildead_title. The title is user provided input and has to be sanitized before it is printed to the user. See http://drupal.org/node/28984
5. slidead_is_visible(): users don't have much control over where this block appears. Just a suggestion, not a blocker: how about adding a setting for a regular expression for paths where it should be displayed?

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

1. You should add your GIT URL to this page: http://git.drupal.org/sandbox/soncco/1441362.git
2. Parareview does not find any automated errors.
3. I would suggest adding a larger image to your project page, and probably more text.
4. I would suggest putting html into template files or template functions, not directly into hook_page_build().

As I did not see any bigger problems, in this not so big project, I put "reviewed & tested by the community.

Code too short

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project.

manual review:

1. Screenshot is big enough if you click on it, that should be ok.
2. "'access arguments' => array('Administer Slide Ad'),": this does still not match the permission name 'administer slidead'.
3. "$is_visible = (isset($menu_item['path']) && (stripos($menu_item['path'], 'admin') === FALSE));": you should use path_is_admin() instead.
4. As there is also a Javascript file I think that this module is long enough to approve you as Git vetted user.

As the permission thing is not a security problem (no one but user 1 can have access to the administration page currently), I think this is ready to go.

Thanks for your contribution, soncco! 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.