Reduce code, break up big functions and move markup to theme functions/templates

Furthermore some theme functions can be called more directly.

There seems to be no need for the existence of function theme_flickr_filter_photo and function theme_flickr_filter_photoset in flickr_filter.module as they only point to their counterparts in the main module without any additions.

Even less.

http://drupalcode.org/project/flickr.git/commitdiff/e71282f

More to come.

Attached patch:

• removes duplicate code from flickrfield.module, using the functions theme_flickr_photo instead of theme_flickrfield_photo and theme_flickr_photoset instead of theme_flickrfield_photoset
• removes 'Source: Flickr' under the set as the caption already links to the Flickr photo page
• opens the photo in colorbox/lightbox instead of linking to the node when in a teaser, as this is the expected behaviour from a visitor's perspective. They can click the post title or 'read more' link to go to the node.

Reroll against latest dev and commit.

http://drupalcode.org/project/flickr.git/commitdiff/2d4ce60

There are flickr requests implemented in some sub-modules that could and should make use of flickr.api.inc instead.

Searching for "flickr_request" (case sensitive)

/home/martin/www/red/sites/all/modules/flickr/block/flickr_block.module:
  910  
  911          // Get a list of "all" the photos in the photoset. This is cached.
  912:         $response = flickr_request('flickr.photosets.getPhotos',
  913            array(
  914              'photoset_id' => $photoset_id,
  ...
  965        default:
  966          // Get a list of "all" the photos in the group. This is cached.
  967:         $response = flickr_request('flickr.groups.pools.getPhotos',
  968            array(
  969              'group_id' => $group_id,
  ...
 1016        default:
 1017          // Do a photo search for the provided tag in the given user's public photos.
 1018:         $response = flickr_request('flickr.photos.search',
 1019            array(
 1020              'user_id' => $nsid,
 ....
 1071          }
 1072  
 1073:         $response = flickr_request('flickr.photosets.getPhotos',
 1074            array(
 1075              'photoset_id' => $photoset_id,

/home/martin/www/red/sites/all/modules/flickr/sets/flickr_sets.module:
   40    // TODO: Why this called for /flickr and does not show for admin role?
   41    if (is_numeric($sid)) {
   42:     return flickr_request('flickr.photosets.getPhotos',
   43        array(
   44          'photoset_id' => $sid,

5 matches across 2 files

Another one is the repeating of identical code snippets. We could create functions for this in flickr.inc.

An example:

Searching 24 files for "$username = !empty(" (case sensitive)

/home/martin/www/red/sites/all/modules/flickr/block/flickr_block.module:
  511        if (!empty($user->flickr['nsid'])) {
  512          $info = flickr_people_get_info($user->flickr['nsid']);
  513:         $username = !empty($info['realname']['_content']) ? l($info['realname']['_content'] . t("'s"), $info['profileurl']['_content'], array('attributes' => array(
  514              'title' => t('View user on Flickr.'),
  515              'target' => '_blank',
  ...
  567          )));
  568        // Real name if it exists or go with the username. Link to Flickr user page.
  569:       $username = !empty($info['realname']['_content']) ? l($info['realname']['_content'], $info['profileurl']['_content'], array('attributes' => array(
  570            'title' => t('View user on Flickr.'),
  571            'target' => '_blank',
  ...
  591          )));
  592       // Real name if it exists or go with the username. Link to Flickr user page.
  593:       $username = !empty($info['realname']['_content']) ? l($info['realname']['_content'], $info['profileurl']['_content'], array('attributes' => array(
  594            'title' => t('View user on Flickr.'),
  595            'target' => '_blank',
  ...
  607        $media = $settings['media'] == 'videos' ? t('videos') : t('photos');
  608       // Real name if it exists or go with the username. Link to Flickr user page.
  609:       $username = !empty($info['realname']['_content']) ? l($info['realname']['_content'], $info['profileurl']['_content'], array('attributes' => array(
  610            'title' => t('View user on Flickr.'),
  611            'target' => '_blank',
  ...
  652        $user = flickr_people_get_info($info['owner']);
  653       // Real name if it exists or go with the username. Link to Flickr user page.
  654:       $username = !empty($user['realname']['_content']) ? l($user['realname']['_content'], $user['profileurl']['_content'], array('attributes' => array(
  655            'title' => t('View user on Flickr.'),
  656            'target' => '_blank'
  ...
  677        $user = flickr_people_get_info($info['owner']);
  678       // Real name if it exists or go with the username. Link to Flickr user page.
  679:       $username = !empty($user['realname']['_content']) ? l($user['realname']['_content'], $user['profileurl']['_content'], array('attributes' => array(
  680            'title' => t('View user on Flickr.'),
  681            'target' => '_blank'
  ...
  697        $media = $settings['media'] == 'videos' ? t('videos') : t('photos');
  698       // Real name if it exists or go with the username. Link to Flickr user page.
  699:       $username = !empty($info['realname']['_content']) ? l($info['realname']['_content'] . t("'s"), $info['profileurl']['_content'], array('attributes' => array(
  700            'title' => t('View user on Flickr.'),
  701            'target' => '_blank',
  ...
  733        $media = $settings['media'] == 'videos' ? t('videos') : t('photos');
  734       // Real name if it exists or go with the username. Link to Flickr user page.
  735:       $username = !empty($info['realname']['_content']) ? l($info['realname']['_content'], $info['profileurl']['_content'], array('attributes' => array(
  736            'title' => t('View user on Flickr.'),
  737            'target' => '_blank',

/home/martin/www/red/sites/all/modules/flickr/flickr.module:
  349    $description = !empty($info['description']['_content']) ? str_replace('"', "'", htmlspecialchars_decode(strip_tags($info['description']['_content']))) : $title;
  350    // Real name if it exists or go with the username. Link to Flickr user page.
  351:   $username = !empty($info['owner']['realname']) ? l($info['owner']['realname'], 'https://www.flickr.com/photos/' . $info['owner']['nsid'], array('attributes' => array('title' => t('View user on Flickr.'), 'target' => '_blank'))) : l($info['owner']['username'], 'https://www.flickr.com/photos/' . $info['owner']['nsid'], array('attributes' => array('title' => t('View user on Flickr.'), 'target' => '_blank')));
  352    // The date an image was taken formatted as 'time ago'.
  353    $taken = isset($info['dates']['taken']) ? format_interval(time() - strtotime($info['dates']['taken']), 1) . ' ' . t('ago') : '';

9 matches across 2 files

To solve #9.

http://drupalcode.org/project/flickr.git/commitdiff/82c2e8b

Solved #10.

http://drupalcode.org/project/flickr.git/commitdiff/f80fb11

See also https://drupal.org/node/383792#comment-8684683 to unify rendering functions even more and as a bonus offer new features.

In #2253641: Extend parameters for group and user IDs. we reduced over 10 functions from flickr_block.module to 3 that moved to flickr.inc because Flickr Filter uses then now as well. The new functions are:

• function _flickr_photoset()
• function _flickr_user()
• function _flickr_group()

Note that the function names are as before but with _block, _random and _recent being removed.

We could reduce them to just one 'super' function _flickr_album(), using a switch statement that produces an array of queried Flickr media ($response based on ID), filters by tags (and for groups by media type) if found (probably using http://www.php.net/manual/en/function.array-filter.php ?), outputs the images, generates a counter if set and caches the whole bunch with a unique key if the calling function is a Flickr Block (not necessary for Flickr Filter as a node body is cached anyway).

• $photoset_id, $nsid and $group_id would all simply become $id.
• $nsid should be removed from function _flickr_photoset(). We can get this info on-the-fly. DONE.
• One additional parameter $type would be used with the value 'set', 'user' or 'group'.

#18 is fixed in http://drupalcode.org/project/flickr.git/commitdiff/a294347.

5 files changed, 86 insertions(+), 134 deletions(-)

Combined 3 filter callbacks into 1.

Now that we have the function _flickr_album() that generates albums for Flickr Filter and Flickr Block, we could possibly extend this for albums displayed on user profiles as well.

It would solve #2171289: Extend Colorbox/Lightbox support to User Profiles. It can only be done after solving #2096881: Use a pager on a embedded photoset in the body (thus 'postponed') as this functionality is already present on profiles. We might even borrow some code for it from there.

Attached patch:

• removes obsolete require_once functions
• replaces the other ones with Drupal's own module_load_include() function
• removes some legacy CCK mentions.

Not to forget #24.

Furthermore some functions have grown too big over time. An example is function _flickr_album() in flickr.inc that consists of almost 1000 lines of code. Some parts of it could be moved into other functions, for example where a link to the Flickr page with the results similar to the album is created.

Each function should perform a specific task. If within that task you find a part that could be a task on itself that might in the future be useful to be called from other function as well, then move it out of the main task (function) and call it from there. It probably facilitates also a D8 port that will likely use classes where appropriate. That follows this principle even more in the form of 'extends' where parent variables can be inherited.

Some parts of it could be moved into other functions, for example where a link to the Flickr page with the results similar to the album is created.

Done in #2457275: Match Flickr results exactly when linking to Flickr in the album title and footer.

We could additionally try to convert some appropriate functions (album and photo constructs) into classes, also as preparation for a D8 port.

Too much markup in the module now. To do some refactoring to use theme functions and/or templates instead.