Project page:
https://drupal.org/sandbox/2pha/1760094

git:
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/2pha/1760094.git link_favicon_formatter
(I hope this was the correct link)

Overview:
Just a simple display formatter for the link module.
It adds the link domains favicon image in front of the link.
After installing the module there should be a 'link with favicon' format option on the link field display options.

Dependencies:
Drupal 7.x
http://drupal.org/project/link

Pareview
http://ventral.org/pareview/httpgitdrupalorgsandbox2pha1760094git

Reviews of other projects
http://drupal.org/node/1764302#comment-6425534
https://drupal.org/node/1778426#comment-6451694
http://drupal.org/node/1781882#comment-6479886

https://drupal.org/node/1806866#comment-6733596 - (didn't review code so not sure if it counts)
https://drupal.org/node/1865074#comment-6847298
https://drupal.org/node/1856530#comment-6849286

Comments

mariooo’s picture

Manual review:

  1. link_favicon_formatter.module:68: Line unused
  2. Should probably mention in README & Project page that you're relying on Google Shared Stuff (s2) for pulling in host favicons. This feature appears to be undocumented by Google – what happens if Google discontinue the service?
  3. What are the conditions under which favicons are shown? My personal site has a favicon (via Drupal's custom favicon mechanism), but it doesn't show via Google s2 (default favicon shown instead).

Other thoughts:

The Project application checklist has a point about having 'a minimum of handwritten code', the community may find it difficult to assess your application as it mainly consists of hook implementations.

There appears to already be an issue for the Link module regarding having favicons next to external links, and a maintainer has said he'd happily include that in a release if a D7 patch was provided.

Perhaps this would be better than a standalone module for Link, going along with Drupal's collaboration rather than competition ethos.

2pha’s picture

Thanks for the review.

I am not quite sure why some icons show and others don't, it suspect it may depend on the 'shortcut icon' meta value being an absolute url.

I did think about patching the link module, but I needed this for my own site and it was easier to create a module. It was only an after thought to submit it to drupal.org. It seems to be a fine line, when to patch a module and when to create a module that extends functionality of another. It seems that everyone has a different opinion about this.

I would like to know others opinions on whether this should be a patch or a module. I don't mind either way, but as I have been a drupal developer for about 4 years, I thought it was time to get a module reviewed so I can start contributing.

2pha’s picture

I just committed the couple of problems outlined by marioo.
I also changed the way the favicon services are handled, to allow for more services.
I also added another service which seems to work more reliably than the google one, there is a new select box to select which one you want to use in the field display settings.

2pha’s picture

Issue summary:View changes

added pareview link

2pha’s picture

Issue summary:View changes

adding Reviews of other projects

lolandese’s picture

Main issue remains that the 3rd party services rely on a favicon.ico being present in the site's root. That can either be:

  • not present
  • the wrong one (mostly the one representing the company where the webite is hosted).

More reliable seems to be the image <link rel="shortcut icon" href="/some-path/my-fav.png"> in the site's HTML. To use this one there are several methods. Attached you find a patch for your .module file using this one (slightly modified). You can see the results here. Feel free to use it or use any other method.

It can be improved by expanding the used RegEx to cover more use cases (e.i. Twitter icon gets not parsed because the href is before the rel). Furthermore a check should be build in for links that do not exist (to my surprise the Link module only validates the link format).

Consider to borrow some code from http://drupal.org/project/urlicon (D6 only). Most interresting feature there is the local storage of the icons to avoid unnecessary parsing over and over again.

Another interesting article.

Good job, happy coding.

2pha’s picture

Thanks for the review lolandese,

I was under the impression that the second favicon service I added got around the problem of the favicon not being in the site's root. Did you try the getfavicon.appspot one?

Thanks for the patch, I will apply it tomorrow when I have time.
I'll also look into saving the icons locally and maybe converting them to png if they are ico

2pha’s picture

Committed the above patch.
Changed the logic to retrieve the favicon which also saves a local copy.

2pha’s picture

Issue summary:View changes

added another project review working towards review bonus

2pha’s picture

adding "PAReview: review bonus" tag

klausi’s picture

Status:Needs review» Postponed (maintainer needs more info)

This looks like a feature that could go directly into the link module, there is even an issue for that: http://drupal.org/node/535948

Module duplication and fragmentation is a huge problem on drupal.org. We prefer collaboration over competition, so please get in touch through that link module issue. You should also contact the link module maintainers to discuss your needs and how they could be included.

If that fails for whatever reason please get back to us and set this back to "needs review".

lolandese’s picture

The URL Icon module also seems a good candidate to offer co-maintainership for, based on the similarity in functionality. It would result in a module that adds a favicon both to link fields and to links found in the node body, expanding the usability.

A D7 version is not available yet, so the current maintainer should be more than happy to accept a co-maintainer. The code of the D6 version seems solid. You might better start with a simple patch for that version before converting it to D7. That way you can test first, before moving it further.

lolandese’s picture

Found https://favatar.mention.net/ that always get them right. When multiple icons are found, the first one is returned, by order of preference:

  • meta og:image
  • link rel="image_src"
  • link rel="shortcut icon"
  • implicit favicon.ico

Personally I would dump the other methods and use this one instead. It's slow though, so you definitely need local storage of the icons to avoid unnecessary parsing over and over again.

2pha’s picture

Status:Postponed (maintainer needs more info)» Needs review

lolandese, I have implemented a local scraper. I don't see any reason to remove all of the others in favor of the one you mention. It could be added though. That being said, I am not going to add anything until the module is either approved or incorporated into another module.
With that said, I will not be offering co-maintainership of the URL Icon module as that module is for a different purpose and has no D7 version...I don't want to have to port that module just to incorporate features that my module already successfully does.

klausi, I contacted Sun who is one of the maintainers for the Link module and he said he really has only been involved in the port to D8 and he was probably not the one to ask. He did say a little more about the D8 version but I am currently at work with no access to my home email so can not quote what he said. I also tried to contact jcfiala about a week ago and have not received a reply.

This seems to be turning into much more work than I anticipated when I had the thought "hmm, maybe I should share this module"...

klausi’s picture

Status:Needs review» Postponed (maintainer needs more info)

@2pha: you already shared your module, that's what sandboxes are for. For full projects we have to be a bit more picky, because otherwise drupal.org gets flooded and you can't find anything. So in order to make your contribution worthwhile we need to hook you up with the people that maintain the existing projects. That might feel a bit slow and frustrating now, but your patience will be appreciated in the long run.

So I suggest that you post into that link issue I mentioned. Introduce your work and offer to provide a patch for the link module. Specify a deadline (e.g in 1 or 2 weeks) after which you will proceed with a separate full project if there is no response.

So if the integration into link module fails I will happily review this and we can proceed.

2pha’s picture

2pha’s picture

Status:Postponed (maintainer needs more info)» Needs review

It's been over a week now and there seems to have been no activity in the issue I posted in. I still have had no reply to the email I sent to jcfiala.

klausi’s picture

Status:Needs review» Needs work
Issue tags:-PAReview: review bonus

Ok, then let's move this application forward.

manual review:

  1. project page is too short, see http://drupal.org/node/997024
  2. link_favicon_formatter_field_formatter_info_alter(): comment "Get available services." does not make sense?
  3. link_favicon_formatter_field_formatter_info_alter(): why do you use the alter hook and not hook_field_formatter_info()?
  4. theme_link_favicon_formatter_favicon(): do not create image markup yourself, use theme('image', ...) instead.
  5. "'#title' => 'Restrict Size',": all user facing text must run through t() for translation.
  6. "drupal_http_request(check_url($url));": no need to use check_url() here, as you are not printing anything to the user. So no sanitization is necessary here. Also elsewhere when using drupal_http_request(). See also http://drupal.org/node/28984
  7. link_favicon_formatter_get_favicon(): check_url() is not ideal for file names, but it does not hurt either (I haven't found a PHP or Drupal alternative for that purpose, grml). You should always check if $elems['host'] is empty and stop immediately.
  8. link_favicon_formatter_get_favicon(): so once a favicon is fetched it will never be replaced, right? Maybe it should expire after a certain amount of time?

But otherwise this looks nearly ready. I'm setting this back to "needs work" because you have to know when to use sanitization functions for security and where not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

2pha’s picture

Status:Needs work» Needs review

1. Added more description and images to the project page
2. Changed comment to be more descriptive.
3. Changed to use hook_field_formatter_info (not exactly sure why I didn't in the first place)
4. Changed to user theme('image', ...);
5. added t() function around all titles etc.
6. Removed check_url calls
7. Removed check_url, check for $elems['host'].
8. Added a settings page that allows you to set how many days a favicon is cached for. Also added ability to remove favicons individually.

bhosmer’s picture

Status:Needs review» Needs work

After adding a link field in the basic page content type, and selecting to use PHP scraper, I get the following error:

Notice: Undefined index: in link_favicon_formatter_field_formatter_view() (line 170 of /link_favicon_formatter/link_favicon_formatter.module).
Notice: Undefined index: in link_favicon_formatter_field_formatter_view() (line 181 of /link_favicon_formatter/link_favicon_formatter.module).

The Google Icon service doesn't work, and just outputs a generic icon, but the appspot service works great.

This is PHP 5.3.6

2pha’s picture

Thanks for taking a look bhosmer,
Both the google and appspot services supply a generic icon if a favicon is not found. It seems that the google service just looks for "favicon.ico" in the root directory. I'm not exactly sure how the appspot one works, but I did encountered times when it did not work.
The PHP scraper gets around the limitations of both the services....well, I should.
I'm at work atm so will take a look at the errors when I get home tonight.

2pha’s picture

Status:Needs work» Needs review

Undefined index problem fixed. It was caused by selecting 'favicon fomatter' for the display, but not setting any of the options for it. Now, if this is the case it used the local php scraper for the icon and the default link.

bhosmer’s picture

Status:Needs review» Needs work

Yes, that was the case. After I went back and set the options, the error went away. Could there be some type of validation or warning to the user that would notify them that they now need to configure that particular display?

As it is now, if a user elects to use the PHP scraper, they will get an error and have no idea how to fix it.

If this is implemented, I would say it would be ready for RTBC.

bhosmer’s picture

I also added a feature request: https://drupal.org/node/1838262

This wouldn't be a release blocker at all, but just an idea for the option to output a custom CSS class to make theming easier.

2pha’s picture

There is no longer any error, it defaults to the php scraper for the favicon and the default link format.
As it stands now, I wont be adding any more features before this going to a full project. This module has been taking way longer than I expected to get through this application process, especially since I am not even using this module on any of my sites.
If a lot of people want the css class feature, I will add it though.
A theme function is used to generate the output, which is easy enough to override for the time being.

bhosmer’s picture

Status:Needs work» Reviewed & tested by the community

Thanks for that. Marking RTBC.

bhosmer’s picture

Issue summary:View changes

added another project review link

klausi’s picture

Status:Reviewed & tested by the community» Needs work

manual review:

  1. "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
  2. link_favicon_formatter_admin(): why do you use the raw $form_state['input'] and cannot use $form_state['values']? Always use the verified $form_state['values'] where possible.
  3. 'Local PHP based icon scrapper': All user facing text must run through t() for translation.
  4. link_favicon_formatter_get_services(): I think you should use a machine name as array keys. The current keys look like they would fit into a 'label' property.
  5. link_favicon_formatter_admin(): this is vulnerable to CSRF attacks. I can build a form that issues a POST request so that favicon files get deleted. I only have to trick an administrator onto a mailicous page and submit that form automatically with javascript. Form I used to exploit this:
<html>
<form action="http://drupal-7.localhost/admin/config/media/favicon_formatter" method="post" id="link-favicon-formatter-admin" accept-charset="UTF-8">
  <input id="edit-cached-favicons-table-2" name="cached_favicons_table[2]" value="2" class="form-checkbox" type="checkbox">
  <input id="edit-refresh" name="op" value="Remove selected" class="form-submit ajax-processed" type="submit"></div>
  <input name="form_id" value="link_favicon_formatter_admin" type="hidden">
</form> 
</html>

You can see that I do not need any token or similar to get favicon files deleted. That is the reason not use $form_state['input'] and to never do data manipulation in form constructors. That should always be done in form submit handlers. See also http://drupal.org/node/178896

klausi’s picture

Issue tags:+PAReview: security

Forgot to add the security tag.

2pha’s picture

1. Done.
2. Done (see #5).
3. changed (see #4)
4. Changed to use machine name keys and a label.
5. I think I had tried using a submit function but it didn't work, so just did it in the form creation function, not knowing it has security implications. Seems it wasn't working because I had forgot to set rebuild = true on the form. It now uses a submit function and checks form_state['values'].

Thanks again for taking the time to review (and educate me) klausi.

2pha’s picture

Status:Needs work» Needs review

forgot needs review

2pha’s picture

Issue summary:View changes

added new review link

2pha’s picture

Issue summary:View changes

added another review link

2pha’s picture

adding PAReview: review bonus tag

klausi’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-PAReview: review bonus

manual review:

  • "variable_get('link_favicon_formatter_lifespan', '');": all variables defined by your module should be deleted in hook_uninstall().

But otherwise looks RTBC to me now! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

2pha’s picture

Forgot to delete variable = another 3 reviews......harsh.

stBorchert’s picture

Status:Reviewed & tested by the community» Fixed

Thanks for your contribution, Chris!

I updated your account to let you 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

2pha’s picture

Thank you stBorchert.
Also thank you to everyone who reviewed this module and especially to klausi for the in depth reviews and security education :)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

added module review link