[D7] Update External Links

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.

Updated git command and added automated review link.

Hi, some manually code review:

update_external_links.module

• 98 - it's recommended to declare global vars at top of function;
• 103, 104 - unused local vars;
• 117, 118 - seems extra newline here?
• 135 - typo in 'finsh';
• 150/155 - broken tabulation;
• 165 - why do you need this session var?

external_links.admin.inc

• 26 - missing t() for localisation;
• 75/77 - wrong tabulation;
• 80 - looks like yoy should use fetchCol() here.

Common: I think you should use more empty lines into functions for separate independent sections of code. This will increase the readability of the code.

Updated issue summary.

Hello @jain_deepak, below are my reviews.

Automated Review

3 Warnings found: http://pareview.sh/pareview/httpgitdrupalorgsandboxjaindeepak2424979git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[No: Causes] module duplication and/or fragmentation.
2 modules are existing.
External New Tab & External Links. Please update this if this module acts differently.

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

[Yes: Follows] 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.
Manual Reviews

1. $context['sandbox']['progress'] ++; this unary operator doesn't need an extra space.
2. drupal_set_message(t('An error occurred while processing @operation with arguments : @args', array('@operation' => $error_operation[0], '@args' => print_r($error_operation[0], TRUE),)));
   Array spans more than 80 characters so break in smaller parts.

This review uses the Project Application Review Template.

Hi Deepak,

Nice module. I have some suggestions:

1. In function update_external_links_selected_nid you can refine the resultset based on node status, field_data_body has value or not.
This will reduce further processing of unwanted nid's.
2. Instead of using node_load in function update_external_links_process_1, you can use 'EntityFieldQuery'.
3. Line 98 in update_external_links.module, URL protocol is hard coded.

Hi Deepak,

For Point 2, you should use field_attach_presave before field_attach_update.
I think node_load is OK to go with if you want, as you are loading single node each time. EntityFieldQuery can be used to load multiple nodes using entity_load.

Also, one more suggestion, In function update_external_links_input_form_submit, two if's can be changed to if-else
[if (empty($node_ids)) and if (!empty($node_ids))]

Git errors:

• Git default branch is not set, see the documentation on setting a default branch.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/includes/external_links.admin.inc
  ----------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ----------------------------------------------------------------------
   61 | ERROR | [x] Expected 1 space after ELSE keyword; 0 found
  ----------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ----------------------------------------------------------------------
  

• 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. what are the differences to existing projects like https://www.drupal.org/project/external and https://www.drupal.org/project/extlink ? Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please add the differences to the project page. It seems like a much more sensible approach to process the links when they are displayed instead of overwriting content in the database?
2. you are repeating "initial changes" as git commit message all the time. Make sure to provide something more useful, see https://www.drupal.org/node/52287
3. update_external_links_url(): why does this function return? Please add @return docs, see https://www.drupal.org/coding-standards/docs#return
4. update_external_links_input_form(): looks like you want to use "checkboxes" for #type here? See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h... . Then you can also make that element #required, so that you don't need update_external_links_input_form_validate().
5. update_external_links_process_1): you should not call the field_attach_*() functions here but rather node_save(), so that really all hooks are invoked when a node is changed.
6. update_external_links_url(): could you add some inline comments explaining what you are doing here? It seems like you only update links pointing to external sites, correct?
7. update_external_links_input_form(): HTML tags should be outside of t() where possible, like the "p" tags here.

But that are not critical application blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Thanks for your contribution, jain_deepak!

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.