Synopsis

The FlickrUp module allows to upload photos to Flickr.
The module provides its own field type, widget and formatters.
It allow use Flickr photo as a field.

Flickr issue: #124189: Uploading photos to Flickr
Media: Flickr

Differences

The main difference from the other similar modules is uploading photos to Flickr.

Requirements

  • Libraries API
  • phpFlickr
  • Galleria plugin [OPTIONAL. For using Galleria formatter/plugin]
  • Using the module is only possible on site accessible from the Internet, because Flickr sends authorization parameters using the domain of your site.

Preview

Format settings
format settings
format settings
Widget
Upload photo
widget
Uploaded photo
widget
One of formatters
galleria

Installation

  1. Download and install the Libraries API module.
  2. Download phpFlickr library.Unpack and rename the library directory to "phpflickr" and place it inside the "sites/all/libraries" directory. Make sure the path to the file phpFlickr.php becomes: "sites/all/libraries/phpflickr/phpFlickr.php".
  3. Download Galleria plugin. Unpack and rename the plugin directory to "galleria" and place it inside the "sites/all/libraries" directory. Make sure the path to the plugin files becomes: "sites/all/libraries/galleria/galleria.js" and "sites/all/libraries/galleria/galleria.min.js".
  4. Get Flickr API authorization information: API Key and API Secret from here. You should create a Flickr app. In the Authentication Flow set the following options:

    After that, return to the your Drupal site. Set the API Key and API Secret in module settings. Save settings.

Git Repository

git clone --branch 7.x-1.x http://git.drupal.org/project/flickrup.git
cd flickrup

Project

link

Pareview

Pareview

Reviews of other projects

  1. https://www.drupal.org/node/2360537#comment-9345391
  2. https://www.drupal.org/node/2368743#comment-9345557
  3. https://www.drupal.org/node/2356849#comment-9367219
  4. https://www.drupal.org/node/2387823#comment-9406913

Comments

k0teg’s picture

k0teg commented

Thank you for this useful module.

manual review:
Please use the template to describe the project here.
Also I suggest moving js and css files to "js" and "css" folders from "includes".

k0teg’s picture

k0teg commented

Manual review:

  • The module description in .info file is not descriptive.
  • I suggest running through a spellchecker all the comments in the module.
  • Please explain what are you trying to do in:
    /**
 * Implements hook_admin_paths_alter().
 */
function flickrup2_admin_paths_alter(&$paths) {
  if (module_exists("overlay")) {
    $paths['admin/config/media/flickrup2'] = FALSE;
    $paths['admin/config/media/flickrup2/auth'] = FALSE;
  }
}

/**
 * Implements hook_custom_theme().
 */
function flickrup2_custom_theme() {
  if (module_exists("overlay") && current_path() === 'admin/config/media/flickrup2') {
    return variable_get('admin_theme');
  }
}
  • Why do you redirect to the frontpage in flickrup2_auth()?
PA robot’s picture

PA robot commented

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.

gunosov’s picture

gunosov commented

Thank you for this review.

Also I suggest moving js and css files to "js" and "css" folders from "includes".

Fixed

The module description in .info file is not descriptive.

Fixed

I suggest running through a spellchecker all the comments in the module.

A spellchecker not found errors, but spellchecker checks only the spelling but not lexical and other errors :(

Please explain what are you trying to do in

It is a way for disabling module “overlay” for my module’s admistrative pages.
The authorization process on the service Flickr is a multistep. After saving the settings form begins first step authentication. Redirect to: flickr.com/services/auth/?api_key=xxxx&perms=delete&api_sig=xxxx. Authorization parameters are passed as GET parameters. When the module "overlay" is enabled, the redirect to:
flickr.com/services/auth/?api_key=xxxx&perms=delete&api_sig=хххх&render=overlay
“&render=overlay” - in the path causes an authorization error. In addition, the Flickr authorization page does not allow embedding within frames. Maybe, this is not the best way to solve this problem, but I do not know how to do it better.
“&render=overlay” parameter is added in the method overlay_drupal_goto_alter (). I considered the possibility of implementing hook_drupal_goto_alter () in my module for remove parameter for some pages, but this has no effect. Maybe because of the modules bootstrap order.
Module "overlay" is evil.

Why do you redirect to the frontpage in flickrup2_auth()?

flickrup2_auth() is a page callback for link: http://yourdomain/admin/config/media/flickrup2/auth.
This link should be add to Flickr App authentication flow in callback URL. If the first step of the authorization succeeds, the Flickr call this URL with GET parameter "frob". Also, this link is written on the settings page of the module, if the administrator enters this link, it will redirect to the module settings page. If another user tries to click on this link, it will redirect to frontpage. This link is system and necessary for a specific scenario.
Now I've made edits, and for other users to be "access denied"

P.S.
See more about the process of authorization in Flickr API here: https://www.flickr.com/services/api/auth.spec.html

If anyone knows how to make this module better - welcome :)

gunosov’s picture

gunosov commented
Status: Needs review » Needs work
gunosov’s picture

gunosov commented
Issue summary: View changes
gunosov’s picture

gunosov commented
Status: Needs work » Needs review

Fixed reviws's notes. Added theme for the field.

gunosov’s picture

gunosov commented
Issue summary: View changes
gunosov’s picture

gunosov commented
Issue summary: View changes
k0teg’s picture

k0teg commented

The current version looks very good for me.
One more thing. It's better to use drupal_get_query_parameters instead of relying on $_GET.

k0teg’s picture

k0teg commented
Issue summary: View changes
Status: Needs review » Needs work
k0teg’s picture

k0teg commented
Issue summary: View changes

Automated Review

  • Git default branch is not set, see the documentation on setting a default branch.

Review of the 7.x-1.x branch (commit 46b99bb):

  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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.
Source: http://pareview.sh/ - PAReview.sh online service

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
No: Does not follow the guidelines for master branch.
Licensing
No: Does not follow the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. It's better to use drupal_get_query_parameters instead of relying on $_GET.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

k0teg’s picture

k0teg commented
Issue summary: View changes
jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented

Automated Review

[Best practice issues identified by pareview.sh] Git default branch is not set, see the documentation on setting a default branch.

Manual Review

Individual user account
[Yes: Follows] the guidelines for individual user accounts.
No duplication
[No: Causes] module duplication and/or fragmentation. There's for example this module, which solves the similar use-case: https://www.drupal.org/project/flickr
Master Branch
[No: Does not follow] the guidelines for master branch.
Licensing
[Yes: Follows / No: Does not follow] the licensing requirements.
3rd party assets/code
[Yes: Follows] the guidelines for 3rd party assets/code.
README.txt/README.md
[Yes: Follows] the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
[Yes: Follows] the guidelines for project length and complexity.
Secure code
[Yes: Meets the security requirements.]
Coding style & Drupal API usage
  1. HTML isn't in seperated theme files. The HTML output cannot be themed in an appropriate way.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

This review uses the Project Application Review Template.

k0teg’s picture

k0teg commented

I suggest reformatting the issue summary like in here.

gunosov’s picture

gunosov commented

Hi Aleh, Peter,
Thank you for your valuable review comments.

Master Branch
[No: Does not follow] the guidelines for master branch.

Fixed.

Licensing
No: Does not follow the licensing requirements.

  • Module uses the phpFlickr library and Galleria.
    The phpFlickr library is released under the GNU General Public License.
    The Galleria core and one bundled theme (Classic) is released under the MIT license.
  • The code of phpFlickr and Galleria are not embed in module’s code files. These tools where added to the module using the module Libraries API.

Coding style & Drupal API usage
It's better to use drupal_get_query_parameters instead of relying on $_GET.

Fixed.

No duplication
[No: Causes] module duplication and/or fragmentation. There's for example this module, which solves the similar use-case:https://www.drupal.org/project/flickr

At this moment, there are no available modules for Drupal 7 which uploades photos to Flickr.
Please, read this comment: https://www.drupal.org/node/124189#comment-8867697
In my view, no make sence to integrate my module with the module flickr, because of differencies in structure of the stored data.
The module flickrup (https://drupal.org/project/flickrup) was created for Drupal 5 and 6.
I would say that my module is the next generation of the module flickrup.

Coding style & Drupal API usage
HTML isn't in seperated theme files. The HTML output cannot be themed in an appropriate way.

Please, explain this item. For theming I use theme functions defined in hook_theme (). Maybe do you mean this:

if (!_flickrup2_is_authorized()) {
 $form['flickrup2_auth_token'] = array(
   '#prefix' => '<div class="flickrup2-auth-token">',
   'heading' => array('#markup' => '<label>' . t('Auth Token') . '</label>'),
   'image' => array(
     '#theme' => 'image',
     '#path' => drupal_get_path('module', 'flickrup2') . '/images/false.png',
   ),
   'text' => array(
     '#markup' => '<p>' . t('No token') . '<br />' . t('Add to Flickr App authentication flow callback URL: <a href="@url">@url</a> and save this settings form to get your token.', array('@url' => $GLOBALS['base_root'] . '/admin/config/media/flickrup2/auth')) . '</p>',
   ),
   '#suffix' => '</div>',
 );
}
else {
 $form['flickrup2_auth_token'] = array(
   '#prefix' => '<div class="flickrup2-auth-token">',
   'heading' => array('#markup' => '<label>' . t('Auth Token') . '</label>'),
   'image' => array(
     '#theme' => 'image',
     '#path' => drupal_get_path('module', 'flickrup2') . '/images/true.png',
   ),
   'text' => array('#markup' => '<p>' . t('Have a token') . '</p>'),
   '#suffix' => '</div>',
 );
}

but I had already remade, as follows:

$form['flickrup2_auth_token'] = array(
 '#type' => 'markup',
 '#theme' => 'flickrup2_auth_token',
);
….
function theme_flickrup2_auth_token($variables) {
 $element = $variables['element'];
 $output = '';
 $output .= drupal_render_children($element);
 $output .= '<div class="form-item form-type-textfield form-item-flickrup2-auth-token">';
 $output .= '<label>' . t('Auth Token') . '</label>';

 if (!_flickrup2_is_authorized()) {
   $output .= theme('image', array(
     'path' => drupal_get_path('module', 'flickrup2') . '/images/false.png',
   ));
   $output .= '<p>' . t('No token') . '<br />' . t('Add to Flickr App authentication flow callback URL: <a href="@url">@url</a> and save this settings form to get your token.', array('@url' => $GLOBALS['base_root'] . '/admin/config/media/flickrup2/auth')) . '</p>';
 }
 else {
   $output .= theme('image', array(
     'path' => drupal_get_path('module', 'flickrup2') . '/images/true.png',
   ));
   $output .= '<p>' . t('Have a token') . '</p>';
 }
 $output .= '</div>';

 return $output;
}

As far as I know, using the theme functions better than the template files for performance.

I suggest reformatting the issue summary like in here.

Good idea. I'm working on it.

gunosov’s picture

gunosov commented
Status: Needs work » Needs review

edited

gunosov’s picture

gunosov commented
Issue summary: View changes
gunosov’s picture

gunosov commented
Issue summary: View changes
Issue tags: +PAreview: review bonus
k0teg’s picture

k0teg commented
Status: Needs review » Reviewed & tested by the community

Automated Review

Review of the 7.x-1.x branch (commit e65c5f6):

  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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.

Source: http://pareview.sh/ - PAReview.sh online service

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation. In comparison to Flickr module this one provides the ability to upload photos (its primary purpose). There is a very similar module FlickrUp but it looks abandoned and for 5.x, 6.x version of Drupal.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements. Please add the corresponding notes to README file about ALL third party libraries.
3rd party assets/code
No: Does not follow the guidelines for 3rd party assets/code. Please add notes, from where you got the images (in 'images folder') to README file.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. I suggest implementing hook_help().
  2.   if (!empty($item['id']) || (!empty($item['file']) && !empty($item['photo_title']))) {
    return FALSE;
  }
  else {
    return TRUE;
  }

    can be replaced with 

    return !empty($item['id']) || (!empty($item['file']) && !empty($item['photo_title']));
  3. It's better to add JS using #attached instead of drupal_add_js().
  4. Use html_tag render elements instead of manually wrapping variables into
    with hardcoded values.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

gunosov’s picture

gunosov commented

Hi Aleh,
Thank you for your review.

Please add the corresponding notes to README file about ALL third party libraries.

Fixed.

Please add notes, from where you got the images (in 'images folder') to README file.

Images are replaced by images created by me.

I suggest implementing hook_help().

Fixed.

It's better to add JS using #attached instead of drupal_add_js().

Fixed.

Use html_tag render elements instead of manually wrapping variables into with hardcoded values.

Fixed.

gunosov’s picture

gunosov commented
Issue summary: View changes
pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

This sounds like a feature that should live either in Flickr Upload or Flickr project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the either in Flickr Upload or Flickr project issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process. It seems currently both existing modules looking for co-maintainer(s).

If that fails for whatever reason, please get back to us and set this back to "Needs Review".

lolandese’s picture

lolandese commented

See https://www.drupal.org/node/124189#comment-9409337

gunosov’s picture

gunosov commented

Thanks for the reply.
I created the issue "Offering to maintain Flickr Upload" and contacted the current project owner. I'm waiting for response.

Leeteq’s picture

Leeteq commented
Related issues: +#2388331: Offering to maintain Flickr Upload

Great initiative, much needed :-)

The Flickr Upload module seems abandoned, no activity. It does not seem necessary with "extra patience" to get attention for the takeover, just have a look at the dates in the issue queue:
https://www.drupal.org/project/issues/flickrup

Leeteq’s picture

Leeteq commented

Congratulations @gunosov - maintainership of the Flickr Upload module has been transferred :-)

gunosov’s picture

gunosov commented

Thanks. I know :)
dev release of FlickrUp for Drupal 7 is already available.
welcome to https://www.drupal.org/project/flickrup

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Postponed (maintainer needs more info) » Needs review

Cool, so I think we can continue here with reviewing the code in the flickrup repository.

gunosov’s picture

gunosov commented
Title: [D7] FlickrUp 2 » [D7] FlickrUp
Issue summary: View changes

git clone command edited

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

manual review:

  1. flickrup_page_build(): do not use drupal_add_css() here, use #attached with the $page render array that this function should receive. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
  2. why is flickrup_js() in the field.inc file? This is a page callback and not a field hook?
  3. flickrup_module_implements_alter(): why does your hook have to run last? Please add a comment.
  4. "$GLOBALS['base_root'] . '/flickrup/auth'": don't build URLs yourself, use url() instead.
  5. 'Flickr photo (FlickrUp)': all user facing text must run through t() for translation.
  6. flickrup_element_process(): the filter_xss() call is not necessary here, you can trust that everything already set in drupal_get_messages() is already sanitized.
  7. theme_flickrup_photo(): the arg() calls are really wrong in a theme function. All information should already be available in the theme variables, if you need that you should pass it into theme() in the first place.
  8. theme_flickrup_field_photo_preview(): This looks vulnerable to XSS exploits. $photo_metadata['title'] and $photo_metadata['description'] go unsanitized to the table cells. All user provided text needs to be sanitized before printing, make sure to read https://www.drupal.org/node/28984 again. Can you point me to the place in code where you perform the sanitization? If I'm right please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

gunosov’s picture

gunosov commented
Issue summary: View changes

Thank you so much for your review klausi. It was really good things you pointed out that I had missed.

flickrup_page_build(): do not use drupal_add_css() here, use #attached with the $page render array that this function should receive. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

Fixed. I removed hook_page_build() implementation and I added CSS file in flickrup_field_formatter_view() using #attached.

why is flickrup_js() in the field.inc file? This is a page callback and not a field hook?

This is ajax callback for removing flickrupfield when clicked on remove button. In this callback it will replace field items. Main job to delete field item done in flickrup_submit_handler. flickrup_ajax and flickrup_submit_handler are placed in flickrup.field.inc file. I think it is a good place for these callbacks :)

flickrup_module_implements_alter(): why does your hook have to run last? Please add a comment.

The authorization process on the service Flickr is a multistep. After saving the settings form begins first step authentication. Redirect to: flickr.com/services/auth/?api_key=xxxx&perms=delete&api_sig=xxxx. Authorization parameters are passed as GET parameters. When the module "overlay" is enabled, the redirect to:
flickr.com/services/auth/?api_key=xxxx&perms=delete&api_sig=хххх&render=overlay
“&render=overlay” - in the path causes an authorization error. “&render=overlay” parameter is added in the method overlay_drupal_goto_alter (). I implemented hook_drupal_goto_alter() and hook_module_implements_alter() in module flickrup for removing that parameter for flickrup's administrative page.

"$GLOBALS['base_root'] . '/flickrup/auth'": don't build URLs yourself, use url() instead.

Fixed.

'Flickr photo (FlickrUp)': all user facing text must run through t() for translation.

Fixed.

flickrup_element_process(): the filter_xss() call is not necessary here, you can trust that everything already set in drupal_get_messages() is already sanitized.

Fixed.

theme_flickrup_photo(): the arg() calls are really wrong in a theme function. All information should already be available in the theme variables, if you need that you should pass it into theme() in the first place.

Fixed.

theme_flickrup_field_photo_preview(): This looks vulnerable to XSS exploits. $photo_metadata['title'] and $photo_metadata['description'] go unsanitized to the table cells.

Fixed.

gunosov’s picture

gunosov commented
Status: Needs work » Needs review
pushpinderchauhan’s picture

pushpinderchauhan commented
Assigned: Unassigned » pushpinderchauhan

Assigning to myself for next review, which will hopefully be tonight.

pushpinderchauhan’s picture

pushpinderchauhan commented
Assigned: pushpinderchauhan » Unassigned
Status: Needs review » Reviewed & tested by the community

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

Review of the 7.x-1.x branch (commit e96f96c):

  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

theme_flickrup_field_formatter(): Still arg() exist, comment needed because arg() is evil, and should almost always be avoided.

flickrup_ajax(): why do you access $_POST here? Please add a comment.

But that are not blocking issues.

Also, Blocking issues from #31 have been addressed. I also tested this module functionality w.r.t XSS, CSRF and Sql Injection and it worked as expected. Good Job!

Been sitting at RTBC for a while now, so...

pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, Anton Hunosau!

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:

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.

Status: Fixed » Closed (fixed)

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