This is a bit technical and arcane, but I want to mention it because I’ve seen a lot of recent patches still using the old style of code calling $sth->finish() explicitly.  I know a lot of the codebase still does this internally, but for Koha such usage is almost always wrong.

If this were just my personal contention, I would couch this in terms of an RFC to deprecate the usage.  But we don’t get a choice here: DBI is telling us “don’t do it”.

Please refer to the DBI perldoc:

When all the data has been fetched from a SELECT statement, the driver should automatically call finish for you. So you should not normally need to call it explicitly except when you know that you’ve not fetched all the data from a statement handle. The most common example is when you only want to fetch one row, but in that case the selectrow_* methods are usually better anyway. Adding calls to finish after each fetch loop is a common mistake, don’t do it, it can mask genuine problems like uncaught fetch errors.

That last sentence is the most concrete and important (hence my bolding).  Moral of the story: don’t do it.  Or if you think you have to, at least comment why, like:

$sth->finish();  # FIXME: leaving data unfetched, should rework query

Recently, I’ve been asked to add tagging to the guided reports section. I used this as an opportunity to implement a mixin that’s similar in spirit to acts_as_taggable for Ruby’s ActiveRecord ORM.

I called this class, “C4::Taggable”, and it will inject tagging-related methods to any class that uses it. Normally, it’s bad form to use Exporter in OO code, but that mainly applies to when you’re defining a class. However, when you’re defining a mixin, I believe the use of Exporter is justified, because it’s one of the easiest ways to inject methods into a namespace.

To illustrate this technique, here is an outline of how C4::Taggable is implemented. (For brevity’s sake, the method bodies were omitted.)

C4::Taggable

  package C4::Taggable;
  use strict;
  use warnings;
  use base 'Exporter';
  use C4::Context;
  our @EXPORT_OK   = qw( add_tag remove_tag tags search_by_tags                );
  our %EXPORT_TAGS =   ( mixin => [qw(add_tag remove_tag tags search_by_tags)] );

  sub add_tag        { }
  sub remove_tag     { }
  sub tags           { }
  sub search_by_tags { }

  1;

Notice how C4::Taggable exports tagging-related methods.

Next, C4::Report is outlined. Note that by using C4::Taggable, C4::Report became taggable by virtue of having tagging-related methods for C4::Taggable being mixed into it.

C4::Report

  package C4::Report;
  use strict;
  use warnings;
  use C4::Context;
  use C4::Taggable ':mixin';

  sub new            { }
  sub id             { }
  sub borrowernumber { }
  sub date_created   { }
  sub last_modified  { }
  sub savedsql       { }
  sub last_run       { }
  sub report_name    { }
  sub type           { }
  sub notes          { }
  sub update         { }

  sub table          { }

  1;

Isn’t that simple? Now any time you want to add tagging to a class, you just use C4::Taggable.

What’s the catch?

The catch is that C4::Taggable will have expectations about the host class and the database schema. The host class has to provide a few methods so that C4::Taggable can introspect enough to generate the right SQL. There will also be a table you have to create that has a predefined structure and naming pattern. I won’t go into any more detail here, but suffice it to say that for C4::Taggable to work, certain conventions have to be followed.

To me, it’s a small price to pay, because it makes it ridiculously easy to add tagging to other classes should that ever be necessary or desired.

One tool I make heavy use of during development is ack. It is a grep replacement that is optimized for searching through source code, and it has been a great help to me for when I needed to dig through the  koha code.

Acks strength is in its really useful set of default behaviors.  Unlike grep, its default is to recurse through your directories while simultaneously ignoring backup files, core dumps, and source control directories like RCS, CVS, .svn, etc.   Technically, you could do this with grep, find, and lots of command-line options, but getting the right incantation might take some trial and error.

Ack condenses all that into 3 easily-typed letters and then gives you a bit more.  One nice touch is that it can use color to highlight matches that it finds, but if it notices that it’s output is being sent to a pipe, it’s smart enough to disable color.  You can also use it to only look through files of the types you care about.  For example, if you wanted to only search through your css and javascript files for the string ’sidebar’, you’d type:

ack –js –css ’sidebar’

There’s also a really handy vim plugin for ack that lets you search through your source tree from within vim.  The search results are presented to you inside a :copen window and you can move backwards and forwards through the list of results using the :cprev and :cnext commands.

I like it a lot, and I recommend giving it a try if you haven’t already.  It certainly makes searching through large codebases a lot easier.

_   /|
\'o.O'
=(___)=
   U    ack --thpppt!

A REPL is an interactive shell for programming languages that:

• Reads expressions,
• Evaluates them,
• Prints the result and
• Loops back to do it all over again

REPLs are simple tools that I’ve found tremendously useful for learning new languages and exploring unfamiliar code. (more…)

Today I sent an RFC about turning on warnings in all of Koha’s Perl scripts and modules. Of course, turning on warnings by itself does nothing except fill up the Apache log; the trick is to quell them by fixing the underlying problems.

As an example, consider misc/migrations_tools/bulkmarcimport.pl. On the plus side, it already contains a use warnings; statement; on the minus side, it was commented out. So close!

After enabling warnings and running bulkmarcimport -d -file test.mrc, we get

deleting biblios
Use of uninitialized value in pattern match (m//) at \
 misc/migration_tools/bulkmarcimport.pl line 118.

One of the most common errors you after turning on warnings are complaints about uninitialized variables. So what’s the variable in question?

if ($format =~ /XML/i) {

OK, so what is $format and why is it uninitialized?

my ($version, $delete, $test_parameter, $skip_marc8_conversion, $char_encoding, $verbose, $commit, $fk_off,$format);

$|=1;

GetOptions(
    'commit:f'    => \$commit,
    'file:s'    => \$input_marc_file,
    'n:f' => \$number,
    'o|offset:f' => \$offset,
    'h' => \$version,
    'd' => \$delete,
    't' => \$test_parameter,
    's' => \$skip_marc8_conversion,
    'c:s' => \$char_encoding,
    'v:s' => \$verbose,
    'fk' => \$fk_off,
    'm:s' => \$format,
);

$format starts off with an undefined value when it is declared by my, and if you don’t supply the -m switch when running bulkmarcimport.pl, it stays undefined.

Fortunately, the script’s usage tells us what the default value of $formatshould be

  m      format, MARCXML or ISO2709 (defaults to ISO2709)

Thus, we can fix that particular warning with this patch:

--- a/misc/migration_tools/bulkmarcimport.pl
+++ b/misc/migration_tools/bulkmarcimport.pl
@@ -2,7 +2,7 @@
 # Import an iso2709 file into Koha 3

 use strict;
-#use warnings;
+use warnings;
 #use diagnostics;
 BEGIN {
     # find Koha's Perl modules
@@ -30,7 +30,8 @@ use IO::File;
 binmode(STDOUT, ":utf8");

 my ( $input_marc_file, $number, $offset) = ('',0,0);
-my ($version, $delete, $test_parameter, $skip_marc8_conversion, $char_encoding, $verbose, $commit, $fk_off,$format);
+my ($version, $delete, $test_parameter, $skip_marc8_conversion, $char_encoding, $verbose, $commit, $fk_off);
+my $format = "ISO2709";

 $|=1;

We’re not done with bulkmarcimport.pl, but this is a start. More anon.