[D7] Link Meta Display Filter

Hi ram4nd,

Please go through the issues pointed out by pareview.sh site and make the necessary changes.

Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /var/www/drupal-7-pareview/pareview_temp/linkmeta.install
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
  5 | ERROR | The third line in the file doc comment must contain a description
  | | and must not be indented
  --------------------------------------------------------------------------------
  
  FILE: /var/www/drupal-7-pareview/pareview_temp/linkmeta.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
  100 | ERROR | Whitespace found at end of line
  105 | ERROR | Whitespace found at end of line
  --------------------------------------------------------------------------------

• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /var/www/drupal-7-pareview/pareview_temp/linkmeta.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
  69 | WARNING | Do not use the raw $form_state['input'], use
  | | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  
  FILE: /var/www/drupal-7-pareview/pareview_temp/linkmeta.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
  109 | WARNING | Unused variable $img_tag.
  --------------------------------------------------------------------------------

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.

A few thoughts:

- I would highly recommend providing an example of what the output looks like, e.g. an image showing a link with an icon and a file size appended to it, so that people get a sense of what their content will look like with the module installed.

- As noted above, the module needs a README, and a big part of that needs to outline the steps to install (which should include specifically that the filter needs to be applied to a text format or nothing will change).

- It would be nice if the images that are uploaded in the admin interface were actually displayed, instead of just the file names and a generic "image file" icon next to them.

- The second checkbox label has a misspelling (should be "extension," not "extentsion"), and since the first checkbox says "file size" I would recommend this say "file extension" for consistency.

- I would recommend putting the help text at the top further down, below the checkboxes, since it's only helpful if the user is actually interested in uploading those icons.

- On your project page, I think you could clarify some of the language under "features." I'm not sure what "Apply the features of this module to text formats using filters," communicates, for instance, other than the fact that this is a text filter (which is stated above, and in the project name). "Icons appear to administration as new icons are displayed in content" doesn't make a lot of sense to me either -- by "administration" do you mean "administrators"?

Hope that helps!

- There is no need to delete the variable cache explicitly in the hook_uninstall. The cache is already deleted in the variable_del() function
- in the _linkmeta_process function you call variable_set('linkmeta_types', $types); for each processed url. This is a huge performance issue because the variables cache is deleted every time when the function is called. I suggest you to call variable_set only is needed.
- I am not sure that you handle all the links that should be skipped. For instance you skip only links that starts with “malito” and “#” but there are many more cases that should handled - @see http://en.wikipedia.org/wiki/URI_scheme.

I will change the status to Needs work only because the performance issue.

Hi ram4nd,

I have some comments about the fixes that you've made regarding my review:

- in the _linkmeta_process function you call variable_set('linkmeta_types', $types); for each processed url. This is a huge performance issue because the variables cache is deleted every time when the function is called. I suggest you to call variable_set only is needed.

Now the cache is not delete for each url but it is deleted per every field which could happen a couple of times on node view. I think that fix should be just to update the variable only when you really have new types found and if you just move the variable set call into this "if" block

      if (!in_array($extension, $types)) {
        $types[] = $extension;
      }

it will be enough.

- I am not sure that you handle all the links that should be skipped. For instance you skip only links that starts with “malito” and “#” but there are many more cases that should handled - @see http://en.wikipedia.org/wiki/URI_scheme.

With this fix you introduce a bug and now only links starting with http and https will pass. But what about urls like /node/1 which dont have a url scheme?
Also the hash check become redundant.

I will change the status because the new introduced bug and the performance issue.

Confirm that issues reported by me are fixed. You have GO from me!

Cheers :)

Same here, everything I mentioned looks good. Since two of us have reviewed you, I'm going to go ahead and set this to RTBC, and see what official Project reviewers think.

Thank you for reviews. Next time please make sure to look at projects in the "needs review" state, as those in "needs work" are blocked anyway and need a response from the applicant.

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

• Codespell has found some spelling errors in your code.
  

  ./linkmeta.module:11: exept  ==> except
  ./linkmeta.admin.inc:21: extention  ==> extension
  

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. linkmeta_init(): why do you need hook_init()? This will run on every single page request, regardless if you display an icon or not. Why can't you add the CSS when you actually replace stuff during text format processing? Ah, the processed text is cached, so then the CSS would not be added ... still, I would recommend hook_page_build() instead which seems more appropriate then hook_init().
2. "$size = filesize(trim($a->getAttribute('href'), '/'));": so this will not work if Drupal is installed in a subdirectory and the href is a relative path, right? I think you need to take the global $base_path into account.
3. You should use format_size() instead of calculating the file size on your own.
4. linkmeta_admin_page(): do not call theme() here, just add the image as nested render array. Drupal will render it later for you.

But otherwise looks good to me, so ...

Thanks for your contribution, ram4nd!

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.