hadyos [hadyos]

In Drupal.org CVS the only accepted files are the ones licensed under GPL license v2+; compatible licenses are not accepted.
Files available from third-party sites should not be included in Drupal.org CVS.

I wonder if you could add your css in fbphotos.page.inc instead of using hook_init and adding it on every page load?

Whilst reading the Drupal API, it was mentioned that hook_init is usually the place were all the CSS and JS files are included for a given module.

The CSS files added in hook_init() are added to all the pages; rather than checking the page being viewed, it is better to add the CSS files in the menu callback for the pages that need those CSS files.

I haven't looked real deep to see if this might be handled elsewhere, but at first glance, it looks like there may be unsanitized user-generated URLs being output, for example in the block. User-generated content (including data supplied by Facebook API) should be run through check_plain(), or filter_xss(), or in the case of URLs, l().

Other than that, a visual review of the code shows that hadyos has a good understanding of the Drupal API and best practices, including security considerations, with the possible exception of the above.

IMHO, this can be RTBC once the above is addressed, and someone actually installs the module to ensure that it doesn't obviously break anything.

The file LICENSE.txt needs to be removed; Drupal.org CVS doesn't allow to commit that file.

nice module! :)

your module uses db_rewrite_sql() which doesn't work with i18n

1. t('Delete album %name', array('%name' => check_plain($album->name))
   

   When the code uses %-placeholders, there is no need to pass the argument to check_plain(), because that is already done by t().

2. The call to db_rewrite_sql() is not correct; without further arguments, the function takes the query is for the nodes table, which is not true in this case. If there are no modules that implement hook_db_rewrite_sql() to alter the table created by your module, it's probably that calling db_rewrite_sql(), or not doesn't make any differences.
3. Strings used in the user interface should be in sentence case.
4. There are functions that don't have the correct name.

Out of the points I reported, the point about the function names is the one that needs to be fixed, to get this application approved.

I am not a developer but I used your module to test. I run into some issues to display the albums to all other users except me.

the bug in in sites/all/modules/fbphotos/fbphotos.page.inc on line 9 is in:
$result = pager_query(db_rewrite_sql($sql), variable_get('fbphotos_albums_limit', 10), 0);

I changed that into:
$result = pager_query(($sql), variable_get('fbphotos_albums_limit', 10), 0);

And it worked :)

Installed this today following the VERY good instructions in the readme.txt; very easy to install.

I'm not a developer or coder so I can only relay my experience and mention a couple of issues:

Thumbnails link back to www.example.com/fbalbums/00000000 which fails if Drupal is installed in a sub-directory i.e. www.example.com/cms/fbalbums/00000000

I had issues when synchronising Albums with Facebook. The first synch goes OK but any subsequent synch just hangs. I found I needed to clear my recent cookies and and then the synch works.

Great module.

Problem seems to be at line 220 :

$img_link = check_url($base_url . "/fbalbums/{$photo->aid}/photo/{$photo->pid}");

$img_link = check_url($base_url . "/cms/fbalbums/{$photo->aid}/photo/{$photo->pid}");

as a quick fix I added my sub-directory to the path.

The code should use url() to obtain the URL.

I am changing the status as per previous comment.

Changing line 220

from:

$img_link = check_url($base_url . "/fbalbums/{$photo->aid}/photo/{$photo->pid}");

to:

$img_link = url($base_url . "fbalbums/{$photo->aid}/photo/{$photo->pid}");

fixes the problem for me.

Not quite fixed, since this introduces another error:

We are yet to import any photos ;) continues to be displayed in the thumbnail block.

line 227

$content .= "<li>We are yet to import any photos ;)</li>";

I guess I'll leave it to the coders.

From a usability point of view it would be useful to be able to control the maximum display size of the original photo. I've been using the default 'html viewer' and since the user has no control over the size of the original Facebook photos invariably some of the photos are quite large, this results in horizontal scroll bars.

I've not had time to install and use the other viewers.

@#28 Realised I could do this by theming fbphotos_photo.tpl.php

I have used fbphotos over several days and not had any major issues until I installed fbconnect (http://drupal.org/project/fbconnect) module.
Both modules operate OK except when in order to refresh the album contents I use the fbphotos 'Synchronise All' button. I then receive the following error message:

Fatal error: Cannot redeclare class FacebookRestClientException in /home/*******/public_html/*******/cms/sites/all/modules/fbphotos/facebook-client/facebookapi_php5_restlib.php on line 3784

Disabling the fbconnect module fixes the issue (I have tried this several times, just to confirm).

Wasn't sure which issue queue to post this in but since hadyos is wanting reviews I've posted the issue here.

@hadyos #31

One other thing I noticed is the thumbnail block only ever selects images in landscape mode. I looked at the code, though not being a coder I was unsure whether this is by design, since the code seems to be selecting images with a greater width than height.

The Facebook PHP API could become an issue since I guess more Facebook / Drupal modules will gradually come on stream.

This is a great module, does exactly what I require, and I personally see no reason not to release it.

There have not been replies in the last week. I am marking this application as won't fix.

Subscribe

Hi everyone,

Is this project still alive? I'm looking for a way to import photo albums from a public page to my Drupal website and this module is the only one I could find that can do that.

However, after following the instructions in the README file, I have this error on facebook.com:

Error
An error occurred with [facebook application name]. Please try again later.

API Error Code: 100
API Error Description: Invalid parameter
Error Message: Requires valid next URL.

Has anybody had this error before?

Thanks,

Blaiz

Hi Hadyos,

I finally got it working, I had to add a next URL on the Facebook application, then add it on my profile and synchronize. It works really well, all the albums are here, I added lightbox, that's really cool.

I really hope it can become a hosted module on Drupal and receive updates because we really need such a module.

However there are some functionalities that we need also. We would like to customize the title of the photo albums. I changed the name from "Album - " to "Photo album: ", by directly changing it in the files but I think it would be cool to be able to do it through a GUI. Another nice feature would be to use the album title in the URL, and make URLs more customizable in general (e.g. instead of /fbalbums/14546451468, it would be cool to have /san-francisco-photos/north-beach).

I will look into it, see how hard it could be to implement that, but I have never worked on a module source code before so I don't know if I'll be able to do it myself.

Thanks

Ok I finally got working what I wanted to do.

The name of the album is now appended in the URL. I used a pathauto function to do that and it works pretty well. That's a bit better for SEO. You need the pathauto module installed and enabled to use this function.
Here is the code, in the fbphotos.page.inc file, after line 17:

    $album['link']        = check_url($base_url . "/fbalbums/{$album_src->aid}");
	
	// Append the title of the album in the URL
	module_load_include('inc', 'pathauto', 'pathauto');
	if (module_exists('pathauto') && function_exists('pathauto_cleanstring')) { 
		$album['link'] .= '/'.pathauto_cleanstring("{$album_src->name}");
	}
		  
    $album['name']        = check_plain($album_src->name);

and you get a URL like youwebsite.com/fbalbums/987646749648_1894651/your-album-title-inserted-here

@blaiz: can you confirm an issue with FBConnect and this module?

@hadyos: did you try to host this module at drupal.org?

I'm not using the FBConnect module.

Hi,

I have this all working but I'm at a loss as to how to display the galleries on the frontend?!?

Am I just missing something glaringly obvious?

Many thanks and great module!

Regards,

Rob.

(Maybe this isn't the place to post this?)

Oops! I found it! It was glaringly obvious!

awesome!

Good luck

Hi Hadyos,

What a great module and something that is really necessary in the drupal module arsenal. All the sites i am going to do want to have there photos on there site aswell and i cant understand why this is not being accepted. Is there any site or place that people can email to try and enforce the module acceptors to take notice.

Great work,
Steve

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

The user who applied for the CVS account didn't reply from April 2010, and on May 14 the application has been closed. Since then, the OP has not uploaded new versions of the code, even if he got reports about what should be changed.

I am closing this application.