Consider implementing Libraries API

Here's an untested patch to add support for Libraries API v2.

Well, you're probably using version 0.9.3. The current version of the library is actually 0.10.1. You can go download the current version from http://timeago.yarp.com/jquery.timeago.js. If that works for you, you can mark this issue RTBC.

Okay, try this one.

I don't really get why the version has to be hard-coded either, but that's what the core hook_library() mandates.

+++ b/timeago.install
@@ -9,24 +9,69 @@
+  // Path to jquery.timeago.js
+  if (module_exists('libraries')) {
+    $path = 'sites/all/libraries/timeago/jquery.timeago.js';
+  }
+  else {
+    $path = drupal_get_path('module', 'timeago') . '/jquery.timeago.js';
+  }

There is no guarantee that the library is in sites/all/libraries. The Libraries module checks several locations. The right way to check if a library is installed is to call libraries_load() and check the "installed" key of the returned array. Also check out libraries_get_path().

+++ b/timeago.install
@@ -9,24 +9,69 @@
-    'title' => $t('Time ago library'),
-    'value' => '0.9.3',
+      'title' => $t('Time ago library'),

extra indenting here

+++ b/timeago.install
@@ -9,24 +9,69 @@
+    if ($update_version > $version) {

Use PHP's version_compare() function.

+++ b/timeago.install
@@ -9,24 +9,69 @@
+      $requirements['timeago']['description'] = $t('It appears that a newer version of this library is available. You may want to download the latest version from <a href="http://timeago.yarp.com/jquery.timeago.js">http://timeago.yarp.com/jquery.timeago.js</a> and overwrite the current version located at ' . "@path.", array('@path' => $path));

Why are you putting "@path" in a separate string? It's not a PHP variable so it doesn't need to be in double quotes, and concatenating strings confuses the translation script.

+++ b/timeago.install
@@ -9,24 +9,69 @@
-    $requirements['timeago']['description'] = $t('Library does not exist. <a href="http://timeago.yarp.com/jquery.timeago.js">Download the library</a> and put it in the timeago module folder.');
+    $requirements['timeago']['description'] = $t('Library does not exist. <a href="http://timeago.yarp.com/jquery.timeago.js">Download the library</a> and put it at ' . "@path.", array('@path' => $path));

Same comment as above -- putting "@path" in a separate string doesn't make sense

+++ b/timeago.install
@@ -9,24 +9,69 @@
 /**
+ * Returns param version of a file, or false if no version detected.
+ * @param $path
+ *  The path of the file to check.
+ * @param $pattern
+ *  A string containing a regular expression (PCRE) to match the
+ *  file version. For example: '@version\s+([0-9a-zA-Z\.-]+)@'.
+ */

Add a line between the description and @params. Also add an extra space before the @param descriptions (there should be 3 spaces after the *) and add "@see libraries_get_version()".

+++ b/timeago.install
@@ -9,24 +9,69 @@
+function timeago_get_version($path, $pattern = '@version\s+([0-9a-zA-Z\.-]+)@') {
+  $version = false;
+  $file = fopen($path, 'r');
+  if ($file) {
+    while ($line = fgets($file)) {
+      if (preg_match($pattern, $line, $matches)) {
+        $version = $matches[1];
+        break;
+      }
+    }
+    fclose($file);
+  }
+  return $version;
+}

This needs to handle the case where the file cannot be reached, e.g. no internet access is available. It also fails if no version string is found.

+++ b/timeago.module
@@ -83,7 +83,7 @@ function timeago_library() {
-      'version' => '0.9.3',
+      'version' => '0.10.0',

latest version is 0.11.1

Thanks

One addition I made in this patch was to define constants for the library website, file name, and download url... was starting to reference them in a number of strings, etc, and it seemed like having constants defined would make things simpler in the future, should the library ever change its location, or file name. I've seen this method used in a number of other modules, but if it's undesirable for some reason, of course I can revert.

Yeah this is fine.

+++ b/timeago.install
@@ -5,28 +5,90 @@
+  if (module_exists('libraries')) {
+    $path = libraries_get_path('timeago');
+    if (libraries_get_path('timeago')) {
+      $path = libraries_get_path('timeago') . '/' . TIMEAGO_LIBRARY_FILENAME;
+    }
+    else {
+      $path = 'sites/all/libraries/timeago/' . TIMEAGO_LIBRARY_FILENAME;
+    }
+  }
+  else {
+    $path = drupal_get_path('module', 'timeago') . '/' . TIMEAGO_LIBRARY_FILENAME;
+  }

The logic here should be:

• If the libraries module exists, $path = libraries_get_path('timeago');
• If $path is empty (i.e. the library was not found or the libraries module is not installed) then look for the path in the module folder.

There is no circumstance under which you can assume that the path should be in sites/all/libraries/timeago.

+++ b/timeago.install
@@ -5,28 +5,90 @@
-  $exists = file_exists(drupal_get_path('module', 'timeago') . '/jquery.timeago.js');
+  $exists = file_exists($path);
+
+  // If file exists...
   if ($exists) {

Let's just do if (file_exists($path)) instead of using an intermediate variable since we don't use that variable anywhere else.

+++ b/timeago.install
@@ -5,28 +5,90 @@
+    $update_version = timeago_get_version(TIMEAGO_LIBRARY_DOWNLOAD_URL);

We should probably cache this so that we don't have to request a foreign URL every time the Status Report page is loaded.

+++ b/timeago.install
@@ -5,28 +5,90 @@
+      $requirements['timeago']['description'] = $t('No update information was found. If desired, you can manually check <a href="@library_download_url">@library_download_url</a> for a newer version of the library.', array('@library_download_url' => TIMEAGO_LIBRARY_DOWNLOAD_URL));

This message should have some indication that the library was installed, not just that no information was found.

+++ b/timeago.install
@@ -5,28 +5,90 @@
+      $requirements['timeago']['description'] = $t('It appears that a newer version of the library is available. You may want to download the latest version from <a href="@library_download_url">@library_download_url</a> and overwrite the current version located at @path.', array('@library_download_url' => TIMEAGO_LIBRARY_DOWNLOAD_URL, '@path' => $path));

Remove the phrase "It appears that" and so the message starts with "A newer version of the library is available"

+++ b/timeago.install
@@ -5,28 +5,90 @@
+    while ($line = fgets($file)) {
+      if (preg_match($pattern, $line, $matches)) {
+        $version = $matches[1];
+        break;
+      }
+    }

This will still check the whole file for a version string -- it should only check the first 200 characters of the first 20 lines or whatever the libraries module does.

Thanks for your work on this.

Committed #12 to dev with minor modifications to the description strings for the status report page. Thanks!

Oh actually this needs to be backported to 6.x... reopening

The change in #13 causes these notices when the module is installed because the .install file is loaded before the .module file:

Notice: Use of undefined constant TIMEAGO_LIBRARY_FILENAME - assumed 'TIMEAGO_LIBRARY_FILENAME' in timeago_requirements() (line 17 of ...\sites\all\modules\timeago\timeago.install).
Notice: Use of undefined constant TIMEAGO_LIBRARY_DOWNLOAD_URL - assumed 'TIMEAGO_LIBRARY_DOWNLOAD_URL' in timeago_requirements() (line 63 of ...\sites\all\modules\timeago\timeago.install).

Probably we should just load the module file at the top of hook_requirements() if it isn't already loaded.

Committed fix to dev. Took the easy way out.

I'm actually okay with not backporting this to D6.