External Link Overlay

welcome,

You already fixed all coding style issues of automated reports, great ;-)

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also create a README.txt that follows the guidelines for in-project documentation.

Your using core = 7.x twice in your info, remove the second one.

You got to delete used variables by variable_del() in hook_uninstall() in a .install file.

I'm afraid 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.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

For some reason you need to be approved before you can add screenshots directly, but if you upload it at https://drupal.org/node/add/image instead and use img-tags for embedding it on your project page, it works.

1) You should add the git clone command to this queue post. See mine for an example:

http://drupal.org/node/1488292

2) I completely agree with the suggestion to add a screenshot - a great idea.

3) It is good practice for hook_init() not to trigger during Drupal installation - i.e. running an install profile.

http://www.exratione.com/2012/03/always-check-for-variable-getinstall-ta...

/**
 * Implements hook_init().
 */
function my_module_init() {
  // don't run unless the install is complete
  if(variable_get('install_task') != 'done') {
    return;
  }

4) When using hook_form_alter() for single forms, you might as well use hook_form_FORM_ID_alter() and skip the if statement.

/**
 * Implements hook_form_FORM_ID_alter().
 */
function extlink_overlay_form_extlink_admin_settings_alter(&$form, &$form_state, $form_id) {

Neither (3) nor (4) are blocking concerns, I'm pushing it back to needs work for what is discussed in comments number #1-#3 above in this thread.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'd love to see this project resurrected. Subscribing.

Hi Basant,

Here are my reviews :

1. In extlink_overlay.module file please change the comment "Implements hook_form_alter()", as the hook being used is hook_form_FORM_ID_alter().

2. While implementing hook_form_FORM_ID_alter, $form_id argument is missing. (https://api.drupal.org/api/drupal/modules!system!system.api.php/function...)

function extlink_overlay_form_extlink_admin_settings_alter(&$form, &$form_state) 

3. There is a space missing after comma in extlink_overlay.module Line No. 21.
4. As this module contains some configuration settings, so file module.info should contains
the configuration link, its not mandatory but it is included in best practices. :)

Tested on FF, and chrome, working well.

Clean code, well documented, useful module! /approved

My only note for the module is that you probably don't want to use hook_init() at all. That will be run on every (non-cached) Drupal page request. The extlink module uses hook_page_build() to load its javascript and css. You could also declare the js and css file in your .info, and it will be included on every page, then hook_page_build() just has to handle 1 setting. See https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func....

That's not a blocking issue though. I notice this issue was tagged with "single project promote" a while back - is that what you are after basant? If you have any questions about the process, please ask.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Basant, that's totally fine since it was in rtbc before the question was asked.

"Single project promote" - this means your sandbox module is promoted to a full release, but your account is not granted "git vetted user" status.
"Git vetted user" - this means you can publish as many full modules as you like without any approval required.

This application was posted over 2 years ago. It's been RTBC for a while.

Any remaining reasons this shouldn't be promoted to a full project?

@basant - Have you reviewed any other modules yet? That's the only thing I can see that might be holding this back now.

Review Bonus
https://drupal.org/node/1975228

Automated Review

extlink_overlay/extlink_overlay.js
Variable curr_object implicitly declared at line 9
Duplicated jQuery selector (at line 13)
Duplicated jQuery selector (at line 38)
Unneeded comma (at line 60)
Empty tag doesn't work in some browsers (at line 21)

README.txt
Typo: In word 'Insatallation' (at line 16)

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

May: Follows the guidelines for 3rd party code. Many times the source of graphics isn't considered when they are used. Can you please confirm that the ajax-loader and close gifs are usable under GPL?

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

No: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

Place code review here using this format:

• js file: The variables context and settings are passed into attach, however they aren't used. Use them. There's no need to grab settings from the global namespace. And using context will help with performance. e.g. $('#edit-extlink-overlay-popup:checkbox', context)
  

  Drupal.behaviors.extlink_overlay = {
      attach: function (context, settings)

• js file: code comments in JS should abide by the same code comment standards as PHP code. Full sentences that are capitalized, etc.
• (+) .info file shouldn't list the js and css files. Not all pages have external links. No need to add extra js and css to them. Instead add them the same way you add js settings in hook_page_build().

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 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, I have promoted this single project manually to a full project for you.

Visit https://www.drupal.org/project/extlink_overlay. You can now make releases for this project.

Thanks for this great review @heddn

@basant - congratulations :)