Link favicon formatter

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.

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.

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".

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.

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: 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.

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.

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

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.

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.

Thanks for that. Marking RTBC.

added another project review link

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

Forgot to add the security tag.

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.

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:

• 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 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.