» Home › Forums › For Coders › SOPAC ›

bug reports

» Submitted by smaskit on Mon, 11/24/2008 - 4:38pm

I am creating this topic since I suspect there will probably be a fair number of bugs found which are small enough not to justify their own topic. Thus this is a place to report straightforward bugs which one believes won't require discussion.

3/20/2009 - Dev Group Meeting Notes ›
Submitted by smaskit on Mon, 11/24/2008 - 4:47pm.

In sopac_catalog.php, the function sopac_put_request_link contains the line

$link = '/myaccount';

However, myaccount is a URL alias which cannot be assumed will be present in all systems. A system which is not have this alias set up will return a page not found error when one clicks on the "Please log in to request this item" link.

Although one could make the myaccount alias a requirement for setting up the system, I believe it makes more sense to change the above line to read

$link = '/user/login';

Submitted by jblyberg on Mon, 11/24/2008 - 5:05pm.

Ooh, good catch. Keep 'em coming. I think I'll do a bug-fix release before a feature release that will address a number of these issues. Great idea creating this thread.

Submitted by smaskit on Thu, 01/15/2009 - 12:41am.

The same problem is in 3 places in sopac_social.php:

1) In theme_sopac_tag_block() where

$block_suffix = '<br /><br /><a href="/myaccount">Login</a> to add tags.</a>';

should become

$block_suffix = '<br /><br /><a href="/user/login">Login</a> to add tags.</a>';

2) In sopac_review_page() where

$rev_form = '<div class="review-login"><a href="/myaccount">Login</a> to write a review</div>';

should become

$rev_form = '<div class="review-login"><a href="/user/login">Login</a> to write a review</div>';

3) In theme_sopac_get_rating_stars where

$login_string = ' - <a href="/myaccount">Login</a> to add yours';

should become

$login_string = ' - <a href="/user/login">Login</a> to add yours';

Submitted by ejk on Tue, 01/20/2009 - 6:01pm.

Whenever there is a link to display, we should use drupal's l() function. It will automatically use url aliases if they exist, and it will automatically handle drupal paths if you have drupal installed in a subdirectory or if you don't have clean urls enabled.

So instead of

$block_suffix = '<br /><br /><a href="/myaccount">Login</a> to add tags.';

It should be

$block_suffix = '<br /><br />' . l("Login", "user") . ' to add tags.';

If there is a URL alias "myaccount" set for the path "user", that's the path that will be rendered in the link text.

Submitted by smaskit on Tue, 12/09/2008 - 1:40am.

A couple of things about the ratings system.

I don't know if this has to do with the sample data you provided, or how we set up our database to use it, but the column insurge_ratings.group_id had a type of CHAR(12). This is clearly too small, especially since the default for group id is 'YOUR GROUP ID' which is 13 characters long. This was resulting in a truncation which caused failure of the system to prevent multiple votes by a single user. I suggest cranking this up to something like 128, and making it of type varchar.

Related, but much less important, in the submit_rating function of the insurge-client.php file, I noticed that you assign the query string for selecting the count of votes by a user to the variable $sql, but then do not use that variable. Also since you are constructing the string without any sort of tokens, I don't see any reason not to make it cleaner by using double quotes and interpolation. Thus I would suggest replacing the following two lines:

$sql = 'SELECT COUNT(rate_id) FROM insurge_ratings WHERE bnum = ' . $bnum . ' AND uid = ' . $uid . ' AND group_id = "' . $group_id . '"';
$dbq =& $db->query('SELECT COUNT(rate_id) FROM insurge_ratings WHERE bnum = ' . $bnum . ' AND uid = ' . $uid . ' AND group_id = "' . $group_id . '"');

with something like this:

$sql = "
  SELECT COUNT(*)
  FROM insurge_ratings
  WHERE bnum = $bnum AND uid = $uid AND group_id = '$group_id'
";
$dbq =& $db->query($sql);

At least to my eye, the latter is significantly easier to read.

Finally, I have not looked at the code closely enough to check whether this is happening, but I would strongly encourage making sure that any data coming from the browser is validated before it gets put into the database.

Submitted by smaskit on Sat, 01/10/2009 - 1:57am.

Quick update: the same field size problem reported for insurge_ratings.group_id also applies to insurge_reviews.group_id and insurge_tags.group_id

Submitted by smaskit on Thu, 01/15/2009 - 3:12am.

A Note On Form Processing

This is not so much a bug as a recommendation for how to take advantage of Drupal's powerful form-handling system. I will talk about a specific example, but my suspicion is that the suggested change could probably also be made in other places in SOPAC.

When a user goes to pay a fine, they start on the page listing their fines, select the fine(s) they want to pay, and submit that form. This brings them to a second page where they enter payment information, and submit it to pay the fine(s). The payment form is processed by sopac_fine_payment_form_submit().

This works fine if the payment succeeds. However, if the payment fails, the user is sent back to the first page, the one listing the fines, and is shown an error message. This means they now have to go through the entire process again, trying to get it right, without the benefit of seeing what they submitted which failed. I think it would be preferable to return them to the payment form, preserving the data they have entered, and giving them an error message so they can attempt to fix things without needing to fill out the entire form again.

The way to do this is to add a sopac_fine_payment_form_validate() function, and to move almost all of what is currently in the submit function to the validate function. In particular, this means moving both the code that attempts to process the payment, and the error handling code, into the validation function (which I would suggest makes a lot of sense).

If processing of the payment fails, one can use form_set_error() to flag the form as having failed. This allows the validate function to return an error message, and more importantly brings the user back to the same form with all of the data they have already entered into it.

An important aspect of how all this works is that the submit function will not be called if the validate function calls form_set_error(). Since the submit function will only be called if the validation succeeds, it can be used solely for doing processing needed when the payment has succeeded: i.e. just what is in the final else clause of the current submit function.

Finally, if there is a need to take information obtained during the processing of the payment, and preserve that information for use by the submit function, the validate function can store that information in $form_state['storage'] which will be available to the submit function. The one caveat on this is that the submit function needs to unset $form_state['storage'] or else Drupal will believe that we are in the midst of processing a multi-page form, and it will return the user to the form.

Submitted by jblyberg on Sun, 01/18/2009 - 12:48am.

Yes! Thank-you. The fine payment form in particular was giving me fits and starts.

Submitted by smaskit on Mon, 01/19/2009 - 5:55pm.

Stars: Incorrect call to JavaScript

In sopac_catalog.php, the function sopac_catalog_search() includes the following line:

drupal_add_js(drupal_get_path('module', 'sopac') .'/js/ui.stars.js');

However, there is no ui.stars.js in the js folder. My suspicion is that the line should read like this:

drupal_add_js(drupal_get_path('module', 'sopac') .'/js/jquery.rating.js');

Making that change made the rating stars work in my version.

Submitted by smaskit on Mon, 01/19/2009 - 6:23pm.

Quoting array keys:

PHP is quite forgiving when it comes to working with arrays. In particular, it will do the right thing if a key is not quoted in a call to an associative array. For example, the following will work fine:

myArray[myKey] = 7;

However, although it will figure out what to do, it will also throw warning of a sufficiently high level that it will be captured by Drupal's error-handling function.

Although that is not a problem when normally running the program, it is something of an inconvenience when stepping through code in a debugger. Although one can do it either way, I think it is preferable to at least try to remember to use quotes. Thus I would argue for replacing the above with this

myArray['myKey'] = 7;

This is not crucial, but it does make life easier when debugging.

Submitted by smaskit on Mon, 01/19/2009 - 6:22pm.

Undefined Variables:

Similar to the preceding comment, the same problem of invoking Drupal's error-handling function occurs when using undefined variable. Thus, for example, if I try to pass an undefined variable in a function call, like so:

function myFunction() {
  // some code which defines $myArgument
  ...
  if($myArgument > 3) {
    $myOtherArgument = 6;
  }
  myOtherFunction($myOtherArgument);
}

Any time the if-clause is false, I will have to go through the error-handling function when I try to debug the above function. Thus I would suggest initializing $myOtherArgument prior to using them, in this case either prior to the if clause, or in an else clause.

Again, this is not crucial, but it does make life easier when debugging.

Submitted by smaskit on Mon, 01/19/2009 - 6:40pm.

Blocks Setup

Although I have not yet figured out how to fix this, I have noticed that the auto-configuration of blocks is not always working correctly. At least some of the time, we are ending up with "Show on every page except" selected instead of " Show on only the listed pages."

An unfortunate side effect of this is that one can get a white screen of death when going to a page which attempts to display such a block without adequate information for its display.

The one place I have found where this is definitely a problem is in sopac_social.php where the theme_sopac_tag_block function is in the record case, but there is no bnum in the URL. To fix this, I suggest replacing the following line:

$bnum_arr[] = $bnum;

with this:

if (is_numeric($bnum)) {$bnum_arr[] = $bnum;}

Obviously, it would be preferable to figure out how to make the blocks configure correctly, but at least the above presents the white screen of death. Also, I suspect that there are other places where a similar fix could be implemented.

Submitted by smaskit on Mon, 01/19/2009 - 7:48pm.

Code vs. Configuration

As SOPAC matures, and becomes more widespread, I suspect that we will see an increasing number of items moving from being hardcoded to being set using the configuration files.

One possibly non-obvious example is for the text checked for when determining whether a fine has been successfully paid. The original code looks for, "Your payment has been approved" which presumably works for Darien. However, this did not work on another library's system, so I had to change it to "Your payment has been accepted".

As things are currently set up, this required changing the code itself, in the pay_fine function of the connector (iiitools_2006.php), but it would obviously be preferable to be able to set this in a configuration file. As a side note, I could also see handling some such items, such as this one, by having a configuration section at the top of the connector file.

Note: presumably, this would be similar to the currently existing iii_available_token configuration item.

Submitted by smaskit on Mon, 05/18/2009 - 2:42pm.

Ampersand Breaks Search

Thanks to funding from the Palos Verdes Library District, I have implemented a fix for the following problem.

If you type, for example, Angels & Demons into either of the sopac search forms, it will perform a search for Angels, and that is all that will be in the search form when it returns: i.e. it truncates from the ampersand on.

This happens because of problems in Apache with parsing URLs that contain an ampersand (even a properly escaped one--i.e. %26) in the path of a URL. Instead of seeing it as part of the path, Apache sees it as the beginning of a new get argument.

In Drupal, the result is that $_GET contains a non-existent parameter (in this case demons), and more importantly, a value for q which has been truncated just before the ampersand.

Drupal addresses this problem with the drupal_urlencode function. This function double encodes ampersands, and a few other problem characters. Thus an & gets converted to %2526 which Apache will correctly parse.

I have modified the theme_sopac_search_handler function so it now runs user-entered search terms through drupal_urlencode.

Note: there is a related problem which the above will not fix. We have created a JavaScript-based widget which dynamically creates URLs, and submits them. Some of the URLs it generates are for sopac searches, but others go elsewhere. URLs which go to other servers can run into problems if they contain double encoding, and thus we are avoiding that. However, that means that our widget is submitting single-encoded search terms to sopac, and that means it will break if there is an ampersand in the search term.

To fix this, I have a module that implements the init hook, and uses it to fix $_GET['q'] if the original request URI contains an ampersand. Here is the code for that function:

    function mymodule_init() {
        // fix common problem where web server incorrectly parses URL that has & in path
        $parts = explode('?', $_SERVER['REQUEST_URI']);
        $main = $parts[0];
        if (stristr($main, '&')) {
            $main = trim($main);
            if (strpos($main, '/') === 0) {
                $main = substr($main, 1);
            }
            $_GET['q'] = $main;
            $test = urlencode($main);
        }
    }

To implement this for your site, simply create a module, and replace mymodule with your module's name in the function name.

Submitted by smaskit on Wed, 06/24/2009 - 12:11am.

I was testing the saved search feature, and noticed the RSS icon link in the list of my saved searches. That would be cool, but it's broken. The saved search url is

/catalog/search/keyword/mySearchTerm

and the RSS feed's url is that preceded by feed: i.e.

/feed/catalog/search/keyword/mySearchTerm

However, SOPAC doesn't claim any url's which begin with feed, nor does it seem ready to convert search results into RSS.

If somebody has a ready-to-go fix for this, great. In the meantime, I'm commenting out the following line in sopac_saved_searches_page() in sopac_user.php:

$search_feed = theme('feed_icon', '/feed' . $search_arr[search_url], 'RSS Feed: ' . $search_arr[search_desc]);

We can uncomment or replace it when we have a way of providing RSS feeds.

Please post a better solution if you have one. Thanks.