[D7] Splash Block

Hi there,

Running the PAreview checker (http://pareview.sh/pareview) on the project showed up some issues which need to be resolved. They are mainly cosmetic but need to be done to ensure the Drupal coding standards are adhered to.

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 have this module in use on several sites, works great.

Useful module. It works nicely.

In the file splash_block.install on line 73 you have "// Check PHPMailer file integrity."

I would like the possibility of hide the splash by click anywhere outside the div#splash-block-splash, not only on a#splash-block-close. Only idea.

Made a simple module like this the other day and though about releasing it but this one's better! I'll likely switch to yours. ;)

Installs and uninstalls fine.

Code looks clean and organized.

Pop Up blocks work.

I do get the following warning:

Warning: Creating default object from empty value in _splash_block_add_splashes() (line 174 of /Users/calvintyndall/Sites/comdev/sites/all/modules/splash_block/splash_block.module). Backtrace:

_splash_block_add_splashes(Array) splash_block.module:144
splash_block_page_build(Array) common.inc:5713
drupal_render_page(Array) common.inc:2675
drupal_deliver_html_page(Array) common.inc:2563
drupal_deliver_page(Array, '') menu.inc:532
menu_execute_active_handler() index.php:21

Also drupal_uninstall_schema('splash_block'); is not needed.

https://drupal.org/update/modules/6/7#install-schema

I was looking for something exactly like this on two of my sites. Thanks for building it!

I've already installed it on both sites, and it works really well. I performed a review of the code, as well, and everything is organized and structured appropriately. I don't see any flaws in the code. Very nice work!

Automated Review

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. 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. You have to get a review bonus to get a review from me.

Linewrap the README. This should really be called README.txt

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation, but rename it.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes, explcitly tested for XSS.

Coding style & Drupal API usage

The README and project page shuld reflect the dependency on Libraries.

Drupal.behaviors.splashBlock isn't using the context that was passed in.

Have you thought about bailing on jstorage and using localStorage instead?

Why is splashBlockSplash in it's own file? Can't that just be part of the closure in splash_block.js?

There is a growing trend that drush commands to download third-party libraries are in the dl- prefix.

Your drush command may be able to jusy download the single file from GitHub instead of the whole zip. Believe it or now, zip/unzip isn't
installed on a lot of servers. I also think some of the logic won't work on Windows.

Use required fields intsead of validating that they aren't empty.

In _splash_block_add_splashes() don't render out the block. Just make sure a nested render array gets into the page.

There should be better directions for adding to a page. I found it a little counterintuitive.

Setting the title and body (w/ default filtered HTML) to test XSS didn't result in alerts. Good.

In _splash_block_add_splashes(), use drupal_html_id() instead of your own logic to clean it.

_splash_block_add_splashes() should build up a nested render array instead of building up markup.

OK, my review looked good and didn't identify any blocking issues. It was also RTBC for a while without objections, so...

Thanks for your contribution, kvnm!

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.