Round 2. :)

I read through our entire code and found a bunch of things. A lot is simply code-style, missing words, inconsistent wrapping, etc.

Some words to ease review follow.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

FileSize
0 bytes

....and the patch.

tstoeckler’s picture

FileSize
34.18 KB

...ahhhhhhhh

tstoeckler’s picture

+++ README.txt	16 Jan 2011 12:46:31 -0000
@@ -24,6 +24,7 @@ Bug reports, feature suggestions and lat
+* Tobias Stöckler (tstoeckler)

Pretty please... :)

+++ libraries.api.php	16 Jan 2011 12:46:31 -0000
@@ -25,13 +25,6 @@
- *   - version callback: (optional) The name of a function that detects and

@@ -39,6 +32,13 @@
+ *   - version callback: (optional) The name of a function that detects and

The ordering before was:
- version callback
- version
- version arguments
now:
- version
- version callback
- version arguments

+++ libraries.api.php	16 Jan 2011 12:46:31 -0000
@@ -72,15 +72,16 @@
- *     - variant callback: (optional) The name of a function that detects
...
+ *     - variant callback: (optional) The name of a function that detects the

Only a change in the first sentence, the rest is wrapping.

+++ libraries.api.php	16 Jan 2011 12:46:31 -0000
@@ -89,19 +90,19 @@
- *     Naturally, external libraries evolve over time and so do their APIs. In
...
+ *     Naturally, libraries evolve over time and so do their APIs. In case a

In light of #864376: Loading of external libraries, I removed the word 'external' here.

+++ libraries.drush.inc	16 Jan 2011 12:46:31 -0000
+++ libraries.drush.inc	16 Jan 2011 12:46:31 -0000
@@ -54,34 +54,31 @@ function libraries_drush_list() {

I just cleaned this up a bit, and killed one if/else, "Hide deletions" probably helps here.

+++ libraries.module	16 Jan 2011 12:46:31 -0000
@@ -247,15 +247,14 @@ function libraries_detect($libraries) {
-  $name = $library['machine name'];
...
+    $library['library path'] = libraries_get_path($library['machine name']);

It was the only place using $name, so I inlined the variable.

+++ libraries.module	16 Jan 2011 12:46:31 -0000
@@ -274,7 +273,7 @@ function libraries_detect_library(&$libr
-      $library['error message'] = t('The version of %library could not be detected.', array(
+      $library['error message'] = t('The version of the %library library could not be detected.', array(

and below:
Since we now have 'error' and 'error message' IMO it makes sense to make 'error message' more complete sentences.

+++ tests/example/README.txt	16 Jan 2011 12:46:32 -0000
@@ -2,7 +2,7 @@
-Version 2
+Version 1

I don't know why we had version 2, but it doesn't really make any sense.

The text below needed some updating, due to our updated testing method.

Powered by Dreditor.

sun’s picture

Awesome!

+* Tobias Stöckler (tstoeckler)
Pretty please... :)

Holy shit, I didn't add you to the README.txt yet?! I'm terribly sorry! SLOPPYME.

+++ README.txt	16 Jan 2011 12:46:31 -0000
@@ -24,6 +24,7 @@ Bug reports, feature suggestions and lat
 * Daniel F. Kudwien (sun) - http://www.unleashedmind.com
+* Tobias Stöckler (tstoeckler)

Mayhaps at least add a link to your d.o profile? Let's also change my link to my d.o profile, please.

Also, I've just seen that README.txt is not in UTF-8 yet, can you fix that, too?

+++ libraries.api.php	16 Jan 2011 12:46:31 -0000
@@ -126,7 +127,7 @@ function hook_libraries_info() {
-      'pattern' => '/@version (\d+)\.(\d+)/',
+      'pattern' => '/@version (\d+)/',

We should double-check this and provide a valid regex example for matching a most common library version, as this was one of the initial integration problems I found.

For example, jQuery module uses:

      // 1.4.3+: jQuery JavaScript Library v1.4.3
      'pattern' => '/v((?:\d+\.?){2,3})$/',

Though that might be not an optimal example. Wysiwyg editor plugins contain better ones, I think.

Also, as visible in the pasted jQuery lines, we might want to document that it is a good idea to remember the different requirements for regex patterns in inline comments right above the pattern. I've introduced that pattern for Wysiwyg some time ago and it turned out to be really helpful, once you need to adjust the pattern for a new version.

+++ libraries.api.php	16 Jan 2011 12:46:31 -0000
@@ -233,13 +226,11 @@ function hook_libraries_info() {
       // Best practice: Document the actual version strings for later reference.
       // 1.x: Version 1.0
-      'pattern' => '/Version (\d+)\.(\d+)/',
+      'pattern' => '/Version (\d+)/',

d'oh :-D

Powered by Dreditor.

tstoeckler’s picture

Copying some version regular expressions from Wysiwyg CVS for reference.
I still violently refuse to learn regular expressions properly :)

  • OpenWYSIWYG: '@v([\d\.]+)@' (v1.4.7)
  • TinyMCE: '@majorVersion[=:]["\'](\d).+?minorVersion[=:]["\']([\d\.]+)@' (majorVersion:'3',minorVersion:'2.0.1') *
  • Whizzywig: '@Whizzywig v?([0-9]+)@' (Whizzywig 60)
  • WYMEditor: '@version\s+([0-9a-z\.-]+)@' (@version 0.5-rc1)
  • YUIEditor: '@version:\s([0-9\.]+)@' (version: 2.8.2r1)
  • CKEditor: '@version:\'(?:CKEditor )?([\d\.]+)(?:.+revision:\'([\d]+))?@' (version:'3.0.1',revision:'4391' ) *
  • FCKEditor: '@^FCKeditor.prototype.Version\s*= \'([\d\.]+)@' (FCKeditor.prototype.Version = '2.6.6' ;)
  • jWYSIWYG: '@([0-9\.]+)$@' (WYSIWYG - jQuery plugin 0.6)
  • MarkItUp: '@([0-9\.]+)@' (markItUp! 1.1.9)

* This currently does not work with libraries_get_version(), because we only catch the first match.

sun’s picture

I'd suggest to change the current regex for this patch into:

'@version\s+([0-9a-zA-Z\.-]+)@'

and move a focused improvement of libraries_get_version() into a separate issue, including details of #5 and #4.

tstoeckler’s picture

Here we go.

I noted the thing about libraries_get_version() merely for reference.
While I'm sure we'll find ways to improve it in the future, I was actually quite astonished to find out it *should* (famous last words...) work for 7 out of 9 editors.
< offtopic >I was also astonished to find out that WYSIWYG now supports 9 editors, by the way, I hadn't checked in for a while...< /offtopic >

tstoeckler’s picture

FileSize
35.43 KB

Dear lord...

tstoeckler’s picture

Oh, and:

Also, I've just seen that README.txt is not in UTF-8 yet, can you fix that, too?

My editor tells me the file is UTF-8, but I'm not entirely sure how to check/change that.

tstoeckler’s picture

FileSize
38.72 KB

In the meantime, when working on the tests for the callback issue, I noticed that the ordering of $expected and $library in some of the tests was a bit weird, from my point of view.

tstoeckler’s picture

Status: Needs review » Needs work
+++ libraries.module	26 Jan 2011 13:15:25 -0000
@@ -372,6 +370,7 @@ function libraries_load($name, $variant 
+      unset($library['variants']);
     }

Those two lines should be switched around: The 'variants' key should be unset whether a variant was specified or not.

Can't be bothered right now to reroll just for that, in light of me still being unable to figure out the UTF-8 thing.

Powered by Dreditor.

sun’s picture

Status: Needs work » Fixed

Had some troubles to understand #11, but got it in the end. Changed those lines as proposed and also added a comment to explain the unset() for future reference (I'm relatively confident that we're going to revise libraries_load() and that variant handling soonish).

Thanks for reporting, reviewing, and testing! Committed to HEAD. Backported relevant changes to D6 (including README.txt).

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

lpalgarvio’s picture

awesome work guys. congrats :)

tstoeckler’s picture

Status: Fixed » Needs work
+++ libraries.module	26 Jan 2011 13:15:25 -0000
@@ -233,7 +232,7 @@ function libraries_info($name = NULL) {
-    libraries_detect_library($library);
+    $library = libraries_detect_library($library);

@@ -243,19 +242,18 @@ function libraries_detect($libraries) {
- *   libraries_info(), passed by reference.
+ *   libraries_info().
...
-function libraries_detect_library(&$library) {
+function libraries_detect_library($library) {

@@ -335,6 +333,7 @@ function libraries_detect_library(&$libr
+  return $library;

Oops. Sorry, that was an accidental change that I had on a local dev version, that wasn't supposed to sneak in the patch. I really hate the reference passing in libraries_detect_library() (yes, I know, I championed that...), but I deemed that to be an API change, which was not doable at the time. Now that we have a 1.0 version, though, we might as well make that change. I'm for it anyways.
But the change is not complete, it needs to change the calling of libraries_detect_library() everywhere including the tests and libraries_load() (!).

Before:
$library = libraries_info('foo');
libraries_detect_library($library);

Now:
$library = libraries_detect_library(libraries_info('foo'));

Sorry for that, I thought I had reverted that stuff properly before rolling the patch.

Powered by Dreditor.

sun’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev

No big deal, since that change didn't go into 1.x anyway.

At first sight, the change made sense to me. Now that you mention it again, I don't think it's beneficial, since it complicates custom library code; i.e.,

Before:
  $library = libraries_info('foo');
  if (libraries_detect_library($library) && $library['installed']) {
    // Do something.
  }

Now:
  $library = libraries_info('foo');
  if (($library = libraries_detect_library($library)) && $library['installed']) {
    // Do something.
  }

or now:
  if (($library = libraries_detect_library(libraries_info('foo'))) && $library['installed']) {
    // Do something.
  }
tstoeckler’s picture

FileSize
1.48 KB
1.48 KB

Well, maybe it's best to discuss any API changes in another issue. Now that we're free to do that, I think I'll open one and write-up a few alternatives.

Anyway, here is a patch to fix our broken HEAD.
Tests are broken before and work with this patch.

tstoeckler’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Fixed
+++ libraries.module	29 Jan 2011 11:19:44 -0000
@@ -333,7 +333,6 @@ function libraries_detect_library($libra
   $library['installed'] = TRUE;
-  return $library;
 }

I left out this change, so it doesn't matter whether you assign the return value or expect the library to be altered by reference.

Thanks for reporting, reviewing, and testing! Committed to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status: Fixed » Closed (fixed)

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