FBApp

You can use `left quotes` to quote field names in MySQL which might make it happier - you'd have to check with the SQL backends to see if they're happy with that too.

Since this is a framework, it's quite difficult for anyone reviewing the code to see if it works without writing some kind of example code to test it! Somewhat painful.

Could you add an example application module that does some basic thing using the framework without changing anything in the framework?

Are there any sites using this module at present?

git repository full of executable files
Changing file permission masks on Windows

Use module include_load_include instead of require_once/include_once etc.

require_once(dirname(__FILE__) . '/includes/fbapp.fbapi.inc');
require_once(dirname(__FILE__) . '/includes/fbapp.cache.inc');

You have a fbapp_cache and functions to deal with it, but you don't use the Drupal API cache_get/cache_set ? Any reason for this?

includes/fbapp.notifications.inc direct printing of $_GET variable to caller! All inputs should be validated regardless. I can get that to print anything I want by choosing a type that doesn't exist and an empty validation key.

function fbapp_rules_publish_photo($settings) {
  $target = $settings['target'];
  $parameters = array(
    'access_token' => $settings['access_token'],
    'source' => '@'. $_SERVER['DOCUMENT_ROOT'] . base_path() . $settings['source'],

I'm sure there's an API function that can used to find the path of the source much better than that.

sql

Bugger. Oh well. After looking at the function in its entirety, yes, it's impossible for it to be exploited there since you use two static strings as the source based on a boolean choice. Could kill the error by changing the layout, but that's just using a nuke to kill a fly in reality. Leave it as is.

test app

Actually, the submodules fbapp_rules and fbapp_viewsfql use the API and provide its functionality through the Rules' and Views UI modules. I am setting up a demo site at http://fbapp.sophostech.com. If you'd like, I can give you admin access to the backend so you can see how it works.

Showing it up and running in a site with perhaps some links and instructions on how things work would be enough. A simple example app would give people a starting point for using the module though which would potentially help with its uptake.

validation

Fixed, added check_plain() to filter the output. Since that's a callback function to be used exclusively by FB's servers, it never occurred to me to sanitize the output... :)

An amazingly common oversight. Just because something is meant to be used by in one way, doesn't mean that it will be ;)

base path

See comments on this - http://api.drupal.org/api/drupal/includes--common.inc/function/base_path/6 - apparently $_SERVER['DOCUMENT_ROOT'] is not set on all platforms.

cache

Well, the reasons why I implemented a separate cache are mainly flexibility and performance. While in its current form it is just a basic cache system, I have some plans to extend it to make it act more like a proxy to minimize the number of calls to FB's API, to make the most out of the data already in store and keep it updated using subscriptions. In theory, this would somewhat improve page load times as well.

Caching is a good thing, but you really are re-inventing the wheel here. You have re-written cache_get and cache_set and all of the associated cruft which is almost an identical copy of core, except that it doesn't have any of the more advanced capabilities.

If you want to have your own cache table, you can simply copy the existing cache schema into your own and use the cache functions as per normal. eg. to have your own caching table that runs with core, replace your existing schema with

function fbapp_schema() {
  $schema['cache_fbapp'] = drupal_get_schema_unprocessed('system', 'cache');
  return $schema;
}

And then just replace your fbapp_cache_set with cache_set($cid, $data, 'cache_fbapp', $expire)
http://api.drupal.org/api/drupal/includes--cache.inc/function/cache_set/6
http://api.drupal.org/api/drupal/includes--cache.inc/function/cache_get/6

The core caching handles whatever kind of thing you throw at it with whatever unique id you throw at it. It gets cleaned up by cron and the default flush caches. It'll also be tied into drush cc all. Site devs can use cache router to do whatever they want with the cached data rather than having to hack the code to include it.

Unless there is a seriously good reason for not using the core cache caching abilities, it should be swaped out.

Woops! Forgot to mention about cURL.

You need to mention that it is a requirement for installation and hook_requirements to make sure it is around ...

OR

Use drupal_http_request instead. This option has the benefit that it'll be available on all platforms that Drupal runs on. It may not do what you want it to though, in which case you need to hook_requirements AND tell people prior to install about it.

See for a hook_requirements() example specifically for curl_init.

ELC, thanks for doing such a thorough review on this. This project is complicated enough that we could easily spend many more months reviewing it, but I don't think that's necessary. I looked through the main module carefully and skimmed the rest. I see a good understanding of security in Drupal, correct implementation of a wide variety Drupal APIs, and general adherence to coding standards. This thread demonstrates torvall is a responsive maintainer and will resolve any additional issues quickly.

I'm marking this RTBC and will move it to fixed in a few days if no one else objects.

currently there is an empty hook_init() implementation. This hook is called on every page request and wastes resources (even if it is just an empty function call in this case). Please remove it.

klausi, thanks for catching that. I know you've been doing a ton of reviews, so you're aware we have a huge backlog to get through. I think this is one we can move out of the queue, even with issues like that.

Projects don't need to be perfect to be approved. Nearly everything on Drupal.org has bugs, and we're not trying to set a higher bar for new contributors. Though these reviews are focused on projects, we're really approving the maintainer. The application process is here to make sure the maintainer is able to successfully maintain the project going forward. I believe we passed that bar a while back on this application.

Does it sound okay to move additional issues to the project issue queue, trust torvall will addressed the issues as he has every other issue raised in this thread, and approve the application?

Yes, works for me.

Thanks for your contribution, torvall! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.