Hi, Perl's warnings pragma is the cheapest way to find bugs. We already use the strict pragma; now it's time to add warnings. More importantly, it's time to review the messages produced by the warnings pragma and fix the underlying issues. To reiterate, this RFC has two goals: turning on warnings, and fixing all warnings thus reported. Doing the first without the second will simply cause Apache logs to overflow. Ideally, all Perl scripts and modules should have warnings enabled by the time 3.2 is released. I propose the following steps to implement this: 1. Update the coding guidelines to require 'use strict' and 'use warnings'. 2. For any new scripts and modules, require 'use warnings'. Since Koha (per Makefile.PL) requires at least Perl 5.6.1, the 'use warnings' pragma should be used instead of -w. 3. Systematically go through and turn on warnings for each script and module. 4. Unsystematically turn on warnings. I.e., whenever you patch a particular script, consider following up with a separate patch to turn on warnings. I suggest a separate patch because turning on warnings is not enough; you should run the script through its paces and fix any problems reported by the warnings pragma. For modules in C4, note that this can be a big commitment: turning on warnings means testing every function and method. 5. Create a standing blocker bug for 3.2 for adding "use warnings". 6. "no warnings" should not be used absent a detailed comment explaining why warnings should be turned off within a block and why it is not possible to change the code to avoid the warning. It _might_ be acceptable for temporary use while turning on warnings in a big module This RFC can also be found at http://wiki.koha.org/doku.php?id=en:development:rfcs3.2:rfc32_turn_on_warnin... Regards, Galen -- Galen Charlton VP, Research & Development, LibLime galen.charlton@liblime.com p: 1-888-564-2457 x709 skype: gmcharlt
Galen Charlton wrote:
Ideally, all Perl scripts and modules should have warnings enabled by the time 3.2 is released.
1000 applauds. Furthermore, demand that the error_log is *empty* during normal use, i.e. a non debug environment.
I propose the following steps to implement this:
1. Update the coding guidelines to require 'use strict' and 'use warnings'. 2. For any new scripts and modules, require 'use warnings'. Since Koha (per Makefile.PL) requires at least Perl 5.6.1, the 'use warnings' pragma should be used instead of -w.
IIRC, it is trivial to grab warnings.pm and install it if your version of Perl does not have the built-in pragma.
6. "no warnings" should not be used absent a detailed comment explaining why warnings should be turned off within a block and why it is not possible to change the code to avoid the warning. It _might_ be acceptable for temporary use while turning on warnings in a big module
I've only had to turn warnings off once in years of coding Perl. That was for one line of code that created a new method on the fly via introspection. Admittedly a rare thing indeed. "big modules" have more need of "use warnings" IMHO. I'll take on the challenge. Show me a some Perl that requires warnings to be turned off and I'll show you how to fix it :) cheers rickw -- ________________________________________________________________ Rick Welykochy || Praxis Services || Internet Driving Instructor The urge to save humanity is almost always only a false-face for the urge to rule it. -- H.L. Mencken
Galen Charlton wrote:
Ideally, all Perl scripts and modules should have warnings enabled by the time 3.2 is released.
1000 applauds. Furthermore, demand that the error_log is *empty* during normal use, i.e. a non debug environment. I agree, except for query logging. It has been proven many times to me
Rick Welykochy a écrit : that it's usefull : the library tells you "I search this, and got no results, there's a bug in koha". You look in the logs, and see that the search was in fact "this and itemtype=that" or "this and available items only", which returns nothing, as there is no answer. So I think queries should be logged. + that would give us some datas to analyse the queries done by users. We just need the analysis too to be written ;-) -- Paul POULAIN http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc NOUVEAU TELEPHONE : 04 91 81 35 08
Hi, On Fri, Aug 29, 2008 at 11:32 AM, Paul POULAIN <paul.poulain@free.fr> wrote:
I agree, except for query logging. It has been proven many times to me that it's usefull : the library tells you "I search this, and got no results, there's a bug in koha". You look in the logs, and see that the search was in fact "this and itemtype=that" or "this and available items only", which returns nothing, as there is no answer.
So I think queries should be logged.
I agree that query logging is useful, but I don't think it should be done via the Apache error log. Most, if not all, OPAC searches are done via GET requests, so the Apache access log could be used. Alternatively, a search log could be kept in a database table, or written to an external file. However, it should not be done (outside of a debug mode) by warn statements that end up in the error log. Regards, Galen -- Galen Charlton VP, Research & Development, LibLime galen.charlton@liblime.com p: 1-888-564-2457 x709 skype: gmcharlt
Paul POULAIN wrote:
So I think queries should be logged.
+ that would give us some datas to analyse the queries done by users. We just need the analysis too to be written ;-)
Not in a production environment. You are imposing your own debugging requirements on all users of a production system, which is inappropriate IMHO. Onc again, it is pretty well impossible to sift through thousands of SQL queries and find the needle in the haystack that is an important system or error message. Instead, add a debug flag to the system to enable query logging. This is just one of many things that should be optionally logged. Or even send the queries to another log file. Did you know you can ask MySQL to log queries? All queries can be saved to a file for later replay if required. This is far more efficient than asking perl and/or the DBI to log the queries. cheers rickw -- ________________________________________________________________ Rick Welykochy || Praxis Services || Internet Driving Instructor Beauty is only skin deep, but ugly goes clean to the bone. -- Dorothy Parker
Rick Welykochy a écrit :
Paul POULAIN wrote
So I think queries should be logged.
+ that would give us some datas to analyse the queries done by users. We just need the analysis too to be written ;-)
Not in a production environment. You are imposing your own debugging requirements on all users of a production system, which is inappropriate IMHO.
Onc again, it is pretty well impossible to sift through thousands of SQL queries and find the needle in the haystack that is an important system or error message.
Instead, add a debug flag to the system to enable query logging. This is just one of many things that should be optionally logged. Or even send the queries to another log file.
Did you know you can ask MySQL to log queries? All queries can be saved to a file for later replay if required. This is far more efficient than asking perl and/or the DBI to log the queries.
Thanks for your point of view and suggestions. But paul also spoke about zebra queries and they are not logged nor saved into the database at the moment which could be good for storing user queries history but is an other point. And they are not mysql, and sometimes needs to be viewed in order to find what the problem is. Friendly. -- Henri-Damien LAURENT
Henri-Damien LAURENT wrote:
But paul also spoke about zebra queries and they are not logged nor saved into the database at the moment which could be good for storing user queries history but is an other point.
Which could perhaps be considered an audit function. Has consideration been given to having some audit tables in the DB to capture who did what and when? This could go a long way towards (a) logging things and (b) transarency and (c) user tracking, esp. for questions like "who changed the MARC framework last night without my consent?" cheers rickw -- ________________________________________________________________ Rick Welykochy || Praxis Services || Internet Driving Instructor Do you realise that in forty years we'll have thousands of old ladies running around with tattoos and rap music will be the golden oldies? -- Maxine
Rick Welykochy a écrit :
Henri-Damien LAURENT wrote:
But paul also spoke about zebra queries and they are not logged nor saved into the database at the moment which could be good for storing user queries history but is an other point.
Which could perhaps be considered an audit function. Has consideration been given to having some audit tables in the DB to capture who did what and when?
This could go a long way towards (a) logging things and (b) transarency and (c) user tracking, esp. for questions like "who changed the MARC framework last night without my consent?" Not so long since there is already something called C4::Log and important actions can then be logged into database. But it can surely be improved. -- Henri-Damien LAURENT
On Fri, Aug 29, 2008 at 11:32 AM, Paul POULAIN <paul.poulain@free.fr> wrote:
I agree, except for query logging. It has been proven many times to me that it's usefull : the library tells you "I search this, and got no results, there's a bug in koha". You look in the logs, and see that the search was in fact "this and itemtype=that" or "this and available items only", which returns nothing, as there is no answer.
So I think queries should be logged.
Hi Paul (welcome back!) - I just sent an email RFC to this list about using Log4perl to start handling our logging and debugging information. I can see it useful for situations like this. If you'd like to log all of the queries in the opac of one library, you could conceivably edit the configuration file to turn up the logging in that area of the application and get the information you want. You could do this with no changes to the Koha code itself, this making it easier to maintain your logging preferences. You can also easily configure it to log more during those times when you're trying to track down a problem. I encourage you to take a look at it and see if an approach similar to the one I laid out might help us make it easier to get this type of information out of Koha. -Andy
On Tue, Aug 5, 2008 at 1:07 PM, Galen Charlton <galen.charlton@liblime.com> wrote:
Hi,
Perl's warnings pragma is the cheapest way to find bugs. We already use the strict pragma; now it's time to add warnings. More importantly, it's time to review the messages produced by the warnings pragma and fix the underlying issues.
To reiterate, this RFC has two goals: turning on warnings, and fixing all warnings thus reported. Doing the first without the second will simply cause Apache logs to overflow.
Ideally, all Perl scripts and modules should have warnings enabled by the time 3.2 is released.
I propose the following steps to implement this:
1. Update the coding guidelines to require 'use strict' and 'use warnings'. 2. For any new scripts and modules, require 'use warnings'. Since Koha (per Makefile.PL) requires at least Perl 5.6.1, the 'use warnings' pragma should be used instead of -w. 3. Systematically go through and turn on warnings for each script and module. 4. Unsystematically turn on warnings. I.e., whenever you patch a particular script, consider following up with a separate patch to turn on warnings. I suggest a separate patch because turning on warnings is not enough; you should run the script through its paces and fix any problems reported by the warnings pragma. For modules in C4, note that this can be a big commitment: turning on warnings means testing every function and method. 5. Create a standing blocker bug for 3.2 for adding "use warnings". 6. "no warnings" should not be used absent a detailed comment explaining why warnings should be turned off within a block and why it is not possible to change the code to avoid the warning. It _might_ be acceptable for temporary use while turning on warnings in a big module
Excellent idea! It will go a long way toward adding some syntactical law and order to our code. -- Chris Nighswonger LibLime www.liblime.com chris.nighswonger@liblime.com
On Tue, Aug 5, 2008 at 12:07 PM, Galen Charlton <galen.charlton@liblime.com> wrote:
Ideally, all Perl scripts and modules should have warnings enabled by the time 3.2 is released.
I'm surprised we haven't done this yet. I hadn't worked on code that didn't use strict and warnings in years before I started on this. It makes it so much easier for many people to cooperate on a code base. I look forward to making this change and think that we can gradually phase it in on the old code so that it's all safe under warnings by the next release. Thanks for pushing us along, Galen! -Andy
Hi, On Tue, Aug 5, 2008 at 12:07 PM, Galen Charlton <galen.charlton@liblime.com> wrote:
I propose the following steps to implement this: ... 5. Create a standing blocker bug for 3.2 for adding "use warnings".
I have opened bug 2505 for this. Regards, Galen -- Galen Charlton VP, Research & Development, LibLime galen.charlton@liblime.com p: 1-888-564-2457 x709 skype: gmcharlt
Galen Charlton wrote:
Hi,
On Tue, Aug 5, 2008 at 12:07 PM, Galen Charlton <galen.charlton@liblime.com> wrote:
I propose the following steps to implement this: ... 5. Create a standing blocker bug for 3.2 for adding "use warnings".
I have opened bug 2505 for this.
I hope 'use strict' is in there as well. Or in another bug report. cheers rickw -- ________________________________________________________________ Rick Welykochy || Praxis Services || Internet Driving Instructor What have the breathtaking advances in communications technology done for the human mind? Beats me. -- Tom Wolfe
On Tue, Aug 12, 2008 at 12:56 AM, Rick Welykochy <rick@praxis.com.au> wrote:
Galen Charlton wrote:
Hi,
On Tue, Aug 5, 2008 at 12:07 PM, Galen Charlton <galen.charlton@liblime.com> wrote:
I propose the following steps to implement this: ... 5. Create a standing blocker bug for 3.2 for adding "use warnings".
I have opened bug 2505 for this.
I hope 'use strict' is in there as well. Or in another bug report.
If I'm not mistaken, we 'use strict' already. Kind Regards, -- Chris Nighswonger LibLime www.liblime.com chris.nighswonger@liblime.com
Hi, On Tue, Aug 12, 2008 at 12:00 AM, Chris Nighswonger <chris.nighswonger@liblime.com> wrote:
On Tue, Aug 12, 2008 at 12:56 AM, Rick Welykochy <rick@praxis.com.au> wrote:
I hope 'use strict' is in there as well. Or in another bug report.
If I'm not mistaken, we 'use strict' already.
We do, but this practice is not mentioned in coding guidelines. I have updated the wiki to promote 'use strict' and 'use warnings': http://wiki.koha.org/doku.php?id=en:development:codingguidelines#perl Regards, Galen -- Galen Charlton VP, Research & Development, LibLime galen.charlton@liblime.com p: 1-888-564-2457 x709 skype: gmcharlt
participants (6)
-
Andrew Moore -
Chris Nighswonger -
Galen Charlton -
Henri-Damien LAURENT -
Paul POULAIN -
Rick Welykochy