I think that the project_release_download_link() function should be converted to a themeable function. Doing so would make it possible for site administrators to override the function to add additional information to the download link, such as adding Google analytics tracking code to the link.

The attached patch changes project_release_download_link() to theme_project_release_download_link() and changes any calls to the function to use the themeing system.

AC

CommentFileSizeAuthor
#5 project_release_theme_download_link_1.txt3.01 KBaclight
project_release_theme_download_link.txt2.42 KBaclight

Comments

dww’s picture

dww
we/he/they
commented

Hrm, not sure I support this. There's already an admin UI to change the link. Why can't you just use that? Why do you need a theme function?

aclight’s picture

aclight commented

The admin UI only allows changing the URL. Google analytics, as an example, needs an onclick attribute on the link.

So, for example, my function looks like this:

function phptemplate_project_release_download_link($file_path, $link_text = NULL, $as_array = FALSE) {
  if (empty($link_text)) {
    $link_text = basename($file_path);
  }
  $download_base = variable_get('project_release_download_base', '');
  if (!empty($download_base)) {
    $link_path = $download_base . $file_path;
  }
  else {
    $link_path = file_create_url($file_path);
  }
  if ($as_array) {
    return array(
      'title' => $link_text,
      'href' => $link_path,
      // add onclick attribute for google analytics tracking
      'attributes' => array('onclick' => $file_path),
    );
  }
  else {
    return l($link_text, $link_path);
  }
}
dww’s picture

dww
we/he/they
commented

Hrm, I see. Well, making it a theme function is certainly easier than adding a complicated UI for setting link attributes and the like. I'll give this a day or two to sink in and see what I think, but I'll probably commit what you've got here. Maybe the new co-maintainer will have thoughts, too. ;)

As always, thanks for the patch and clear issue title/description...
-Derek

aclight’s picture

aclight commented
Status: Needs review » Needs work

there is also a call to project_release_download_link() in project.module (project_page_overview()) that needs to be converted and added to the patch.

aclight’s picture

aclight commented
Status: Needs work » Needs review
StatusFileSize
new project_release_theme_download_link_1.txt3.01 KB

This patch fixes the call to project_release_download_link() described in comment #4.

NOTE: If applied AFTER the patch at http://drupal.org/node/105224, this patch won't apply because both change the same line.

It should be trivial to fix by hand, though. Or, possibly CVS is smarter than patch and will be able to apply the entire thing without failing.
AC

dww’s picture

dww
we/he/they
commented

Sure, seems reasonable. I'll resolve by hand if necessary...

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

And lo, needed to re-roll it (both for that patch, and the code-style commit). Tested, and committed to HEAD. Don't think I'll backport to DRUPAL-4-7--2.

aclight’s picture

aclight commented

By the way, if anyone wants to actually use this to track downloads using Google analytics, don't use the code I posted in comment #2. Use this code instead (in the template.php).

function phptemplate_project_release_download_link($file_path, $link_text = NULL, $as_array = FALSE) {
  if (empty($link_text)) {
    $link_text = basename($file_path);
  }
  $download_base = variable_get('project_release_download_base', '');
  if (!empty($download_base)) {
    $link_path = $download_base . $file_path;
  }
  else {
    $link_path = file_create_url($file_path);
  }
  if ($as_array) {
    return array(
      'title' => $link_text,
      'href' => $link_path,
      // Add onclick attribute for Google Analytics tracking.
      'attributes' => array('onclick' => "javascript:urchinTracker ('$file_path') ;"),
    );
  }
  else {
    return l($link_text, $link_path, array('onclick' => "javascript:urchinTracker ('$file_path') ;"));
  }
}

The code I posted earlier was missing some crucial parts.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)