On Thu, Jul 9, 2009 at 12:16 PM, Marc Chantreux<marc.chantreux@biblibre.com> wrote:

...

hello koha coders,

Koha naming convention drove us to a stupid bug: $query is used both for the CGI object and the SQL statements.

1. Change the sql statement name a) to $statement b) to $sql c) to $smt d) to an explicit name (as $select_old_borrowers or $update_items ...) e) to your own idea 2. Change the CGI object a) to $cgi (i have no idea why ...) b) to $r (as there is a lot of perl code still using this convention) c) to $input (because a part of koha already use it) d) to your own idea 3. I don't want to change because *you* did errors ... $query is ok for me!

for myself, i'm fan of 1d *and* 2a ($query is too generic and $input has no sense when i write print $input->header).

On 2009/07/10, at 5:06 AM, Kyle Hall wrote:

I would go with 1b and 2a myself.

i agree with 1b and 2a :) yeah, $input and $query are pretty general :/ $cgi and $sql are much more explicit with the exception that $sql could be something more verbose where needed eg: like $sql_all_expired_borrowers etc.. if other people agree, i will try to do this in the future.

I agree with Joe. If we were to adopt a coding guideline out of this, it should be that only C4 modules may call Context->dbh(). And it wouldn't bother me to see CGI objects renamed as $cgi. Ryan 2009/7/9 Joe Atzberger <ohiocore@gmail.com>:

3) Both are valid.

For the most part, SQL should be in C4 modules, and only .pl scripts should have CGI objects.

I am fine with 2a, using $cgi as a variable name, too, but I wouldn't spend much time on this.

-- Joe Atzberger LibLime - Open Source Library Solutions

On Thu, Jul 9, 2009 at 12:16 PM, Marc Chantreux <marc.chantreux@biblibre.com> wrote:

...

hello koha coders,

Koha naming convention drove us to a stupid bug: $query is used both for the CGI object and the SQL statements.

So at the end of an hard week, we write :

my $query = CGI->new; # lines of code $query = "delete * from my_brain"; # lines of code print $query->header;

and try to understand why header is not a function available in the weird "delete * from my_brain" package ...

so we would like to change at least one of these convention to prevent this easy error. As we are for a stronger coding style in koha, we feel that this decision must be debated with all of you.

proposals:

1. Change the sql statement name   a) to $statement   b) to $sql   c) to $smt   d) to an explicit name (as $select_old_borrowers or $update_items ...)   e) to your own idea 2. Change the CGI object   a) to $cgi (i have no idea why ...)   b) to $r (as there is a lot of perl code still using this convention)   c) to $input (because a part of koha already use it)   d) to your own idea 3. I don't want to change because *you* did errors ... $query is ok for me!

for myself, i'm fan of 1d *and* 2a ($query is too generic and $input has no sense when i write print $input->header).

regards

-- Marc Chantreux BibLibre, expert en logiciels libres pour l'info-doc http://biblibre.com

_______________________________________________ Koha-devel mailing list Koha-devel@lists.koha.org http://lists.koha.org/mailman/listinfo/koha-devel

-- Ryan Higgins LibLime * Open-Source Solutions for Libraries Featuring KohaZOOM ILS 888-564-2457 x704

2009/7/9 Kyle Hall <kyle.m.hall@gmail.com>

Is there any way to limit access to Context->dbh to only the C4 Modules? Being able to enforce coding guidelines would be nice.

Kyle

http://www.kylehall.info Information Technology Crawford County Federated Library System ( http://www.ccfls.org )

This might, in theory, be possible. It might be easier to simply excise the offending statements, however. The following quickly written shell command brings up a list of all CGI scripts that reference C4::Context->dbh: $ find -name '*.pl' | xargs grep -l 'new CGI' | grep -v sms | xargs grep -l 'C4::Context->dbh' Some of these entries are due to older, crustier parts of the API that still require passing $dbh, which is fairly easy to fix. The admin/ scripts are the worst offenders, but there is plenty else to keep us busy. (Creating a module like C4::Admin to house the necessary SQL from the admin scripts would be a worthwhile separate discussion.) A quick search on Bugzilla didn't find anything related to this, but I could easily have missed it. If not, we should create a tracking bug for this purpose. -- Jesse Weaver

...

Some of these entries are due to older, crustier parts of the API that still require passing $dbh, which is fairly easy to fix.

Fixing the remaining API calls that require $dbh to be passed would be a good first step.

Several of my patches pending do exactly this.