[D7] Link icons

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxnatts12287455git

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.

Automated review

PAreview is clear as per previous comments.

Manual Review

Duplication

The project title seems to similar to the existing linkicon module: https://drupal.org/project/linkicon. While that module does something different, you should consider a name change to avoid future confusion between link_icons and linkicon.

README.txt

The README file is nice and clear, but doesn't follow the (Admittedly not very firm guidelines - https://drupal.org/node/447604. Consider following the styles set out in that link for headings and bullet points.

I installed the module, and gave it a try. I've attached a patch which makes some changes to the strings in the UI to make the capitalisation consistent with other fields on the "Manage display" screens.

Thanks natts,

I'm going to leave as "Needs Review" as I'm not sure on the policy around potential naming conflicts - other than that I'd be happy to mark as RTBC - hopefully someone else will review either flip the switch - or clarify.

Hello natts.

It seems to me that it
/**
* Field formatter for Link field.
*
* @file link_icons.module
*/
can be replaced to that
/**
* @file
* Field formatter for Link field.
*/

String 246:
The $inverse variable might have not neeb defined.

String 256:
"_blank" - Can you use single quotes?

String 287:
The $text variable might have not neeb defined too. If case is default.

Thanks.

Manual review

link_icons.module file:

• It's better to put endpoint in all translatable string.
• It's a recommendation to use single quotes as default, except when your string have apostrophe. you have several translatable strings with double quotes.
• Your code is without any documentation. How do you expect others to understand what you wanted to do?

Also, none of the items previous identified by Ruslan was resolved.

Good work! ;)

Missing git clone command in your issue. Please provide a git clone command.

Manually review.

Line 43, 111, file link_icons.module, function link_icons_field_formatter_settings_form()

You should use property #empty_option, #empty_value to add empty option to select element.

Line 81, file link_icons.module function link_icons_field_formatter_settings_form().

Notice: Undefined index: fixed-width in link_icons_field_formatter_settings_form() (line 81 of .../sites/all/modules/contrib/link_icons/link_icons.module).
Setting name fixed-width is not defined in hook_field_formatter_info().

[Optional] Line 40, 54, file link_icons.module, function link_icons_field_formatter_settings_form()

String "if any" should have same style with the other. "if any" vs "(if any)"

Automated Review

This is from drush dcd
FILE: ...P/htdocs/drupal7/sites/all/modules/contrib/link_icons/link_icons.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
3 | ERROR | The second line in the file doc comment must be " * @file"
4 | ERROR | The third line in the file doc comment must contain a description
| | and must not be indented
--------------------------------------------------------------------------------

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Only as mentioned above.

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. (That is, no third party code).

README.txt

README is fairly

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

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

Coding style & Drupal API usage

Place code review here using this format:

• Please consider more in-line documentation, especially in the hook_field_formatter_view() function - this will allow more readability.

Hi natts,

Thanks for the contribution. This is a nice module that serves a very useful function.

I tested it. It works.

Automated review shows a few minor problems.

FILE: /var/www/drupal-7-pareview/pareview_temp/link_icons.module
--------------------------------------------------------------------------------
FOUND 3 ERRORS AND 1 WARNING AFFECTING 4 LINES
--------------------------------------------------------------------------------
  3 | ERROR   | [x] Expected 1 space(s) before asterisk; 0 found
  4 | ERROR   | [x] Expected 1 space(s) before asterisk; 0 found
  5 | ERROR   | [x] Expected 1 space(s) before asterisk; 0 found
 87 | WARNING | [ ] Avoid backslash escaping in translatable strings when
    |         |     possible, use "" quotes instead
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Review checklist is all checked.

No need to repeat the previous review. The only problem there is that link_icons_field_formatter_view() could use a few comments. But this is not such a major issue that should prevent the project from being promoted.

A suggestion

I would recommend having a way for other modules to add additional sites without hacking the module code. For instance, in link_icons_field_formatter_view() you could do:

  $services = module_invoke_all('link_icons_services');
  drupal_alter('link_icons_services', $services);

And you could return your services from implementing the same hook:

function link_icons_link_icons_services() {
  return array(
    'behance.net' => array('behance', 'black', 'behance-square'),
    // ...
  );
}

But this is a suggestion, and not a problem you must fix. The only blocking issue, in my opinion, is to fix the PAReview errors.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

I see that the corrections are made. I see a small problem that the function link_icons_link_icons_services doesn't have comment. I suggest to change it to:

/**
 * Implements hook_link_icons_services().
 */
function link_icons_link_icons_services() {

I think this is good enough to be promoted. It already had very minor issues previously, so I think this is overdue. Thanks again for the nice contribution and effort.

Hi Natts,
Sorry about the delay in reviewing your application. I'm assigning this to myself to take a final look and then I will go ahead with the application process :)

There are few modules that provide similar functionality. The most similar ones are:
- https://www.drupal.org/project/link_favicon_formatter
- https://www.drupal.org/project/linkicon

They have slightly different functionality, but linking to them and explaining the differences will make it easier for others.

Thanks for your contribution!

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.