Bug 8798 - Add the use of DBIx::Class
Summary: Add the use of DBIx::Class
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: 3.12
Hardware: All All
: P3 enhancement with 10 votes (vote)
Assignee: Elliott Davis
QA Contact:
URL:
Keywords:
Depends on: 10636
Blocks: 5837 10493 10565 15760
  Show dependency treegraph
 
Reported: 2012-09-20 19:51 UTC by Elliott Davis
Modified: 2016-02-11 10:23 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (427.99 KB, patch)
2012-09-20 21:14 UTC, Elliott Davis
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (429.54 KB, patch)
2012-09-21 01:37 UTC, Elliott Davis
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (430.57 KB, patch)
2012-09-21 13:30 UTC, Elliott Davis
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (430.92 KB, patch)
2012-09-21 16:03 UTC, Elliott Davis
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (430.97 KB, patch)
2012-09-21 16:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8798 - Add the use of DBIx::Class - Rename schema class updater (874 bytes, patch)
2012-09-21 16:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (430.92 KB, patch)
2012-09-22 18:34 UTC, Elliott Davis
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (431.25 KB, patch)
2012-09-24 15:14 UTC, Elliott Davis
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (431.27 KB, patch)
2012-09-24 16:01 UTC, Elliott Davis
Details | Diff | Splinter Review
SIGNED-OFF-Added-base-class-files-for-all-tables-in (431.22 KB, patch)
2012-09-24 16:40 UTC, wajasu
Details | Diff | Splinter Review
SIGNED-OFF-Bug-8798-Add-the-use-of-DBIx-Class-Rename (2.75 KB, patch)
2012-09-24 17:03 UTC, wajasu
Details | Diff | Splinter Review
[Follow-up] Updated to remove tabs (1.54 KB, patch)
2012-09-25 18:37 UTC, Elliott Davis
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8798 - Add the use of DBIx::Class - Rename schema class updater (979 bytes, patch)
2012-09-26 11:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Updated to remove tabs (1.62 KB, patch)
2012-09-26 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
0003-SIGNED-OFF-Bug8798-Updated-to-remove-tabs (1.60 KB, patch)
2012-09-26 18:50 UTC, wajasu
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (431.42 KB, patch)
2013-04-22 11:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8798 - Add the use of DBIx::Class - Rename schema class updater (882 bytes, patch)
2013-04-22 11:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug8798 Updated to remove tabs (1.87 KB, patch)
2013-04-22 11:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (431.29 KB, patch)
2013-04-22 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8798 - Add the use of DBIx::Class - Rename schema class updater (882 bytes, patch)
2013-04-22 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug8798 Updated to remove tabs (1.87 KB, patch)
2013-04-22 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8798 - Add the use of DBIx::Class - Followup - Update schema files (75.62 KB, patch)
2013-06-18 17:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (431.16 KB, patch)
2013-06-18 22:19 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8798 - Add the use of DBIx::Class - Rename schema class updater (943 bytes, patch)
2013-06-18 22:20 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug8798 Updated to remove tabs (1.86 KB, patch)
2013-06-18 22:20 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8798 - Add the use of DBIx::Class - Followup - Update schema files (75.64 KB, patch)
2013-06-18 22:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8798 - Moving code to Koha::Database and adding tests (10.43 KB, patch)
2013-06-18 22:22 UTC, Chris Cormack
Details | Diff | Splinter Review
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context (469.84 KB, patch)
2013-06-26 19:00 UTC, Paul Poulain
Details | Diff | Splinter Review
DBIx::Class Branch.pm implementation (6.99 KB, patch)
2013-06-26 19:00 UTC, Paul Poulain
Details | Diff | Splinter Review
DBIx::Class tests: replacing GetBranchName by it's DBIx::Class equivalent (1.86 KB, patch)
2013-06-26 19:00 UTC, Paul Poulain
Details | Diff | Splinter Review
Testing DBIx::Class, continued (2.95 KB, patch)
2013-06-26 19:01 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Davis 2012-09-20 19:51:09 UTC
This bug will lay the foundation for using DBIx::Class.  It will also include the class files from the automated class generator.
Comment 1 Elliott Davis 2012-09-20 21:14:18 UTC Comment hidden (obsolete)
Comment 2 Elliott Davis 2012-09-20 21:17:14 UTC
This patch adds the Class files for the Koha Database and generated the Schema file for DBIx::Class.  C4::Context has been modified to add a ->schema handle so that new code may take advantage of DBIx::Class.

To Test: 

Install patch
Make sure you can still connect to Koha
Done!
Comment 3 wajasu 2012-09-20 22:58:43 UTC
I need you to add the dependency to koha_perl_deps.pl.
This way we can drive any workflow for derived artifacts (koha-common, etc)

Also, possibly investigate updating any debian packages list.

Note: We may want to let someone benchmark if it does load modules, on each request.

I was thinking we would right a plack based repo service that we can call and keep DBIx out of apps/modules, but maybe that too much, we'll see. I'm excited about this.  Bringing in an ORM.  Got to get the ball rolling.

wajasu

koha@biblio:~/kohaclone$ prove t/Context.t 
t/Context.t .. 1/1 
#   Failed test 'use C4::Context;'
#   at t/Context.t line 9.
#     Tried to use 'C4::Context'.
#     Error:  Base class package "DBIx::Class::Schema" is empty.
#     (Perhaps you need to 'use' the module which defines that package first,
#     or make that module available in @INC (@INC contains: /home/koha/kohaclone /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl .).
#  at /home/koha/kohaclone/Koha/Schema.pm line 9.
# BEGIN failed--compilation aborted at /home/koha/kohaclone/Koha/Schema.pm line 9.
# Compilation failed in require at /home/koha/kohaclone/C4/Context.pm line 102.
# BEGIN failed--compilation aborted at /home/koha/kohaclone/C4/Context.pm line 102.
# Compilation failed in require at (eval 6) line 2.
# BEGIN failed--compilation aborted at (eval 6) line 2.
# Looks like you failed 1 test of 1.
t/Context.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
t/Context.t (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=1,  1 wallclock secs ( 0.02 usr  0.01 sys +  0.06 cusr  0.00 csys =  0.09 CPU)
Result: FAIL
Comment 4 Elliott Davis 2012-09-21 01:37:13 UTC Comment hidden (obsolete)
Comment 5 wajasu 2012-09-21 07:02:13 UTC
I tried again.  I tried to apt-get
libdbic-class-schema-loader-perl
      ^
      x
so i loaded it with an x instead.
I also needed to change DBIC to DBIx in C4/Installer/PerlDependencies.pm

I'll leave it to you to fix.  So I can signoff tomorrow.

After those i tried:
koha@biblio:~/kohaclone$ prove t/Context.t 
t/Context.t .. 1/1 
#   Failed test 'use C4::Context;'
#   at t/Context.t line 9.
#     Tried to use 'C4::Context'.
#     Error:  Undefined subroutine &DBIx::Class::Exception::blessed called at /usr/share/perl5/DBIx/Class/Exception.pm line 55.
# Compilation failed in require at /home/koha/kohaclone/C4/Context.pm line 102.
# BEGIN failed--compilation aborted at /home/koha/kohaclone/C4/Context.pm line 102.
# Compilation failed in require at (eval 6) line 2.
# BEGIN failed--compilation aborted at (eval 6) line 2.
# Looks like you failed 1 test of 1.
t/Context.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 


I believe DBIx::Class::Exception is in   DBIx::Class  (which would have come as a dependency.)  Maybe there is something else?  a DBD... DBI... 
I was playing with carton/cpanfile and did a reverse engineer with just these:
requires 'DBD::mysql', '4.022';
requires 'DBIx::Class::Schema::Loader', '0.07033';
so maybe we need?  No its not those, I have them in my test VM.

Maybe its a PERL5LIB or search path thing?

testdb.pl  - I used this in my  host OS when I was getting familiar with DBIx.

#!/usr/bin/perl

use strict;
use warnings;

use Koha::Schema;

  my $schema = Koha::Schema->connect('DBI:mysql:database=koha;host=192.168.122.70','koha','koha');

  get_biblios();

  sub get_biblios {
    print "get_biblios:\n";
    my $rs = $schema->resultset('Biblio')->search(
    );
    while (my $biblio = $rs->next) {
      print $biblio->title . "\n";
    }
    print "\n";
  }
}
Comment 6 Colin Campbell 2012-09-21 11:12:18 UTC
Can I suggest adding a small script containing the dbicdump command to recreate the schema files. That way anyone in the future who changes the db schema just needs to run this to update the files. Saves effort, confusion and ensures consistency.
Comment 7 Elliott Davis 2012-09-21 13:13:25 UTC
Sure Colin!  Thanks for reminding me.  

Also Wajasu, You're right! I misspelled the package name.  Must've had one too many beers after dinner.  Note to self, never Drink with DBIx :p

Thanks for testing.  I'm also going to include my test file in the next patch so that you guys can use it to test further.

(In reply to comment #6)
> Can I suggest adding a small script containing the dbicdump command to
> recreate the schema files. That way anyone in the future who changes the db
> schema just needs to run this to update the files. Saves effort, confusion
> and ensures consistency.
Comment 8 Elliott Davis 2012-09-21 13:30:56 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-09-21 14:27:08 UTC
This is looking good. A couple comments though.

dummytest.pl output looks good. I don't know if you meant to include this file though.

t/Context.t gives me:

DBIx::Class::Relationship::HasOne::might_have(): "might_have/has_one" must not be on columns with is_nullable set to true (Koha::Schema::Result::Borrower/cardnumber). This might indicate an incorrect use of those relationship helpers instead of belongs_to. at /home/koha/kohaclone/Koha/Schema/Result/Borrower.pm line 780

I'm not sure if this is really an issue or not, though. It seems like a problem with our schema, not with his patch.

genDBIxFiles.sh works, but you have to modify the data in it. It would be nice if it would find the db info in the koha conf file. At the very least, it could ask for the database, username and password.
Comment 10 Kyle M Hall 2012-09-21 14:28:23 UTC
> genDBIxFiles.sh works, but you have to modify the data in it. It would be
> nice if it would find the db info in the koha conf file. At the very least,
> it could ask for the database, username and password.

Also, I'd move genDBIxFiles.sh to something like misc/devel instead of having it in the root directory.
Comment 11 Kyle M Hall 2012-09-21 14:30:15 UTC
> dummytest.pl output looks good. I don't know if you meant to include this
> file though.

Bad wording. What I meant to say is should this file be included in Koha?
Comment 12 Elliott Davis 2012-09-21 16:01:00 UTC
Kyle you're probably right and I probably jumped the gun in my excitement to get this in.  I'll paste the script here: 

#!/usr/bin/perl                                                                                                                                                                                                                               

use strict;
use warnings;
use C4::Context;
use Data::Dumper;

my $schema = C4::Context->schema;

print Dumper($schema->resultset("Borrower"));
Comment 13 Elliott Davis 2012-09-21 16:03:28 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2012-09-21 16:13:56 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2012-09-21 16:17:30 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2012-09-21 16:18:45 UTC
Excellent work! I can't wait to use this!
Comment 17 Colin Campbell 2012-09-21 16:33:26 UTC
Couple of minor anomalies, you're missing classes for labels_batches, labels_templates and labels_layouts They were added via updatedatabase but dont exist in kohastructure.sql 

You've got an ActionLogs class but if you recreate the schema it generates an ActionLog class (Standard behaviour the Loader generates a singular named class for plural tables) shouls ActionLogs.pm be removed?
Comment 18 Elliott Davis 2012-09-21 16:44:10 UTC
(In reply to comment #17)
> Couple of minor anomalies, you're missing classes for labels_batches,
> labels_templates and labels_layouts They were added via updatedatabase but
> dont exist in kohastructure.sql 
So you're sayng they exist in the database because they were added by updatedatabase but not in kohastructure.sql?  

> 
> You've got an ActionLogs class but if you recreate the schema it generates
> an ActionLog class (Standard behaviour the Loader generates a singular named
> class for plural tables) shouls ActionLogs.pm be removed?

Are you saying when you ran the generation script it gave you the singular form?  I see the plural form in the Results file so I'm a bit confused as to how that slipped through as well.
Comment 19 Colin Campbell 2012-09-21 17:08:39 UTC
(In reply to comment #18)
> (In reply to comment #17)

> So you're sayng they exist in the database because they were added by
> updatedatabase but not in kohastructure.sql?
Yes I think so. I wondered if you were generating from a pristine db. (not really a prob for this patch)  
> 
> > 
> > You've got an ActionLogs class but if you recreate the schema it generates
> > an ActionLog class (Standard behaviour the Loader generates a singular named
> > class for plural tables) shouls ActionLogs.pm be removed?
> 
> Are you saying when you ran the generation script it gave you the singular
> form?  I see the plural form in the Results file so I'm a bit confused as to
> how that slipped through as well.
Yes I get a ActionLog.pm which is what I would expect. Using your script or dbicdump.
Comment 20 Elliott Davis 2012-09-21 17:12:07 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> 
> > So you're sayng they exist in the database because they were added by
> > updatedatabase but not in kohastructure.sql?
> Yes I think so. I wondered if you were generating from a pristine db. (not
> really a prob for this patch)  
> > 
> > > 
> > > You've got an ActionLogs class but if you recreate the schema it generates
> > > an ActionLog class (Standard behaviour the Loader generates a singular named
> > > class for plural tables) shouls ActionLogs.pm be removed?
> > 
> > Are you saying when you ran the generation script it gave you the singular
> > form?  I see the plural form in the Results file so I'm a bit confused as to
> > how that slipped through as well.
> Yes I get a ActionLog.pm which is what I would expect. Using your script or
> dbicdump.

It was from my development DB, I'm not sure if that qualifies as pristine ;)
I wouldn't be opposed to a QAA or Paul re-running the gen script and pushing their results.  This would probably resolve both of the aforementioned issues.  The important thing in this patch is less the class files and more the code addition in C4::Context for the schema handler.

Let me know if you'd like me to try and regen or if there is something we can get paul or a QAA to do.
Comment 21 Colin Campbell 2012-09-21 18:50:39 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > 
> > > So you're sayng they exist in the database because they were added by
> > > updatedatabase but not in kohastructure.sql?
> > Yes I think so. I wondered if you were generating from a pristine db. (not
> > really a prob for this patch)  
> > > 
> > > > 
> > > > You've got an ActionLogs class but if you recreate the schema it generates
> > > > an ActionLog class (Standard behaviour the Loader generates a singular named
> > > > class for plural tables) shouls ActionLogs.pm be removed?
> > > 
> > > Are you saying when you ran the generation script it gave you the singular
> > > form?  I see the plural form in the Results file so I'm a bit confused as to
> > > how that slipped through as well.
> > Yes I get a ActionLog.pm which is what I would expect. Using your script or
> > dbicdump.
> 
> It was from my development DB, I'm not sure if that qualifies as pristine ;)
> I wouldn't be opposed to a QAA or Paul re-running the gen script and pushing
> their results.  This would probably resolve both of the aforementioned
> issues.  The important thing in this patch is less the class files and more
> the code addition in C4::Context for the schema handler.
> 
> Let me know if you'd like me to try and regen or if there is something we
> can get paul or a QAA to do.

Just out of curiosity if you remove ActionLogs.pm and regenerate does it create ActionLog.pm or ActionLogs.pm
Comment 22 Elliott Davis 2012-09-21 19:29:44 UTC
I still get ActionLogs.pm

Strange
Comment 23 wajasu 2012-09-21 23:54:17 UTC
Investigated why I get the exception as described in comment #5.

I was able to get the test to run with cpanm with a local::lib running on my hostOS(archlinux).  I compared the DBIx::Class::Exception between my archlinux cpan install (possible metacpan), and my koha test debian VM.

Undefined subroutine &DBIx::Class::Exception::blessed called at /usr/share/perl5/DBIx/Class/Exception.pm line 55

In that file the code is different:
debian code with libdbix-class-schema-loader-perl:
    # Don't re-encapsulate exception objects of any kind
    die $msg if blessed($msg);
my archlinux host cpanm local::lib  has:
    # Don't re-encapsulate exception objects of any kind
    die $msg if ref($msg);

koha@biblio:/usr/share/perl5/DBIx$ grep VER *       DEBIAN SQUEEZE VM
Class.pm:$VERSION = '0.08123';   <-- debian squeeze 
kohaclone/local/lib/perl5/DBIx$ grep VER *          ARCHLINUX hostOS
Class.pm:$VERSION = '0.08200';

The reason I am investigating this is if squeeze's package has an older .deb, it may break folks upgrading, just by installing the package and our upgraded code. Note: Also, my hostOS has a much newer perl 5.16.1.

Maybe its my environment's fault, where I pull my deb packages:
(OR do i need to do ann update in some way) 
Here is my /etc/apt/sources.list
# deb cdrom:[Debian GNU/Linux 6.0.5 _Squeeze_ - Official amd64 NETINST Binary-1 20120512-20:40]/ squeeze main

#deb cdrom:[Debian GNU/Linux 6.0.5 _Squeeze_ - Official amd64 NETINST Binary-1 20120512-20:40]/ squeeze main

deb http://ftp.gtlib.gatech.edu/debian/ squeeze main
deb-src http://ftp.gtlib.gatech.edu/debian/ squeeze main

deb http://debian.koha-community.org/koha squeeze main

deb http://security.debian.org/ squeeze/updates main
deb-src http://security.debian.org/ squeeze/updates main

# squeeze-updates, previously known as 'volatile'
deb http://ftp.gtlib.gatech.edu/debian/ squeeze-updates main
deb-src http://ftp.gtlib.gatech.edu/debian/ squeeze-updates main

I'm documenting this for all future DBIx related deployment.

Also, I had done a dbicdump on my archlinux with the newer version and it generated  Koha::Schema::ActionLog.pm
Maybe yours was generated with the debian pkg version.

wajasu
Comment 24 Elliott Davis 2012-09-22 02:36:23 UTC
Wajasu perhaps it is a package oddity.  Colin what are you testing on?  

Should this prevent this from being signed off on?
Comment 25 wajasu 2012-09-22 06:58:15 UTC
Better news.
I got my fresh debian VM built.  Here are may latest observations:

In the VM:
koha@biblio:~/kohaclone/local/lib/perl5/DBIx$ grep VER *
Class.pm:$VERSION = '0.08200';
So my other VM must have been older (a month, and maybe grabbed an older version).

koha@biblio:~/kohaclone$ prove t/Context.t 
t/Context.t .. DBIx::Class::Relationship::HasOne::might_have(): "might_have/has_one" must not be on columns with is_nullable set to true (Koha::Schema::Result::Borrower/cardnumber). This might indicate an incorrect use of those relationship helpers instead of belongs_to. at /home/koha/kohaclone/Koha/Schema/Result/Borrower.pm line 780
t/Context.t .. ok   
All tests successful.
Files=1, Tests=1,  2 wallclock secs ( 0.04 usr  0.00 sys +  0.78 cusr  0.07 csys =  0.89 CPU)
Result: PASS

(Just guessed that you want DateTime when inflating. You might have not.)
[Maybe you can note the exact dbicdump command line you used in the bug report, for future reference.  I know they might want Versioned, but I know you have started tweaks, probably for cardnumber in borrower.]
koha@biblio:~$ dbicdump -o dump_directory=./lib -o components='["InflateColumn::DateTime"]' -o debug=1 Koha::Schema 'DBI:mysql:database=koha;host=localhost' koha koha
Dumping manual schema for Koha::Schema to directory ./lib ...
Schema dump completed.

I got ActionLogs.pm in my dump in this debian VM.

koha@biblio:~/kohaclone$ prove t/Context.t
t/Context.t .. DBIx::Class::Relationship::HasOne::might_have(): "might_have/has_one" must not be on columns with is_nullable set to true (Koha::Schema::Result::Borrower/cardnumber). This might indicate an incorrect use of those relationship helpers instead of belongs_to. at /home/koha/kohaclone/Koha/Schema/Result/Borrower.pm line 780
t/Context.t .. ok   
All tests successful.
Files=1, Tests=1,  1 wallclock secs ( 0.03 usr  0.00 sys +  0.80 cusr  0.04 csys =  0.87 CPU)
Result: PASS

I also tested to see if DBIx tries to connect to the DB, by shutting down the mysql instance, and it made no difference.

So you might want to get rid of the Borrower/cardnumber  thing above.

We can't have this spitting out junk everytime we use the context. Note: It does so for "perl Makefile.PL" or any thing that uses C4::Context.
Comment 26 Elliott Davis 2012-09-22 18:34:29 UTC Comment hidden (obsolete)
Comment 27 wajasu 2012-09-22 21:01:04 UTC
i found a way to squelch the validation warning. in C4/Context.pm i put
$ENV{'DBIC_DONT_VALIDATE_RELS'} = 1; # FIXME once the DBIx schema has its schema adjusted we should remove this

If this is going out in whatever state its in, this will prevent us from seeing the warning all over the place, but we should write a bug noting this to track it as a possible outstanding issue.  But we will loose this other lrelationship validations as well.  When we write tests, we might just write a test for the 
schema for us to see those validation tests.
See the source for reference.
/usr/share/perl5/DBIx/Class/Relationship/HasOne.pm

prove t/Context.t
t/Context.t .. ok   
All tests successful.
Files=1, Tests=1,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.69 cusr  0.05 csys =  0.78 CPU)
Result: PASS

I ran dbicdump on my debian VM and it matches what Elliot got.  ActionLogs.pm
I ran it on my archlinux hostOS and it generates ActionLog.pm
So maybe some transitive dependency for plural/stemming etc is newer on my, and Colin's environments.  You might want to rename it to ActionLog to align with future behavior.
Comment 28 Colin Campbell 2012-09-24 08:01:47 UTC
(In reply to comment #24)
> Wajasu perhaps it is a package oddity.  Colin what are you testing on?  
> 
> Should this prevent this from being signed off on?

No it shouldnt stop sign off. Looks like its a bug in debian's package.
I was just trying to confirm the behaviour to see if theres a bug in loader.
I'm testing on Fedora using perl 5.16.1/5.17.4 DBIx::Class::Schema::Loader 0.07033
Comment 29 Elliott Davis 2012-09-24 15:14:25 UTC Comment hidden (obsolete)
Comment 30 Elliott Davis 2012-09-24 16:01:48 UTC Comment hidden (obsolete)
Comment 31 wajasu 2012-09-24 16:40:06 UTC Comment hidden (obsolete)
Comment 32 wajasu 2012-09-24 17:03:25 UTC Comment hidden (obsolete)
Comment 33 Mason James 2012-09-25 11:49:07 UTC
(In reply to comment #31)
> Created attachment 12480 [details] [review]
> SIGNED-OFF-Added-base-class-files-for-all-tables-in

failing this patch for new TAB chars in...

C4/Context.pm
C4/Installer/PerlDependencies.pm
Comment 34 Elliott Davis 2012-09-25 18:37:50 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2012-09-26 11:11:29 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2012-09-26 11:17:24 UTC Comment hidden (obsolete)
Comment 37 wajasu 2012-09-26 18:50:14 UTC Comment hidden (obsolete)
Comment 38 Jared Camins-Esakov 2012-10-06 04:26:15 UTC
Changing target version so this bug report will show up in the list of 3.12-targeted features and doesn't get lost in the shuffle during feature freeze.
Comment 39 Paul Poulain 2012-11-28 16:23:33 UTC
wajasu, have you seen my patches on bug 8309 ? I think it's worth a shared effort
Comment 40 Paul Poulain 2012-12-21 13:51:12 UTC
wajasu, ping again !
Comment 41 Elliott Davis 2012-12-21 15:35:07 UTC
While this patch is not an exact duplicate it does have significant overlap.  Enough so as to warrant closing this bug.

*** This bug has been marked as a duplicate of bug 8309 ***
Comment 42 Elliott Davis 2013-02-08 18:46:47 UTC
After closely looking at bug 8309 it appears that this is a more closely integrated solution.
Comment 43 Elliott Davis 2013-02-08 18:57:53 UTC
re-adding the sign-off status from wajasu
Comment 44 Paul Poulain 2013-02-20 10:17:28 UTC
(In reply to comment #42)
> After closely looking at bug 8309 it appears that this is a more closely
> integrated solution.

Sorry Eliott, but I think I'm missing what you mean here. Could you give more details ? Thx
Comment 45 Kyle M Hall 2013-04-22 11:52:29 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2013-04-22 11:52:38 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2013-04-22 11:52:46 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2013-04-22 12:02:24 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2013-04-22 12:02:36 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2013-04-22 12:02:43 UTC Comment hidden (obsolete)
Comment 51 Magnus Enger 2013-05-06 07:18:57 UTC
I think the main objection to using DBIx::Class in Koha has been concerns about speed? It looks like the newest release addresses that: http://jjnapiorkowski.typepad.com/modern-perl/2013/05/perl-dbixclass-an-awesome-orm-now-with-super-speed.html
Comment 52 Paul Poulain 2013-05-06 07:37:46 UTC
(In reply to comment #51)
> I think the main objection to using DBIx::Class in Koha has been concerns
> about speed? It looks like the newest release addresses that:
> http://jjnapiorkowski.typepad.com/modern-perl/2013/05/perl-dbixclass-an-
> awesome-orm-now-with-super-speed.html

The other concern, if i'm not wrong, is that the schema requires a lot of time to load. In persistent mode, it can be ignored, as it's made one.

So we must have plack working before introducing DBIx::Class.
Comment 53 Kyle M Hall 2013-05-06 12:24:18 UTC
I had a conversation with Chris Cormack about this and he didn't raise any such concerns. Maybe we should ask him if he thinks this is not a problem or if it was something neither of us thought about at the time.

Chris's primary concern was that we should always be able to make direct SQL queries when we need to for speed, but DBIx::Class allows for that quite easily. This conversation took place when I wrote my proof of concept using DBIx::SearchBuilder as an alternative. After the conversation I deprecated that patch in favor of using DBIx::Class.

(In reply to comment #52)
> (In reply to comment #51)
> > I think the main objection to using DBIx::Class in Koha has been concerns
> > about speed? It looks like the newest release addresses that:
> > http://jjnapiorkowski.typepad.com/modern-perl/2013/05/perl-dbixclass-an-
> > awesome-orm-now-with-super-speed.html
> 
> The other concern, if i'm not wrong, is that the schema requires a lot of
> time to load. In persistent mode, it can be ignored, as it's made one.
> 
> So we must have plack working before introducing DBIx::Class.
Comment 54 Chris Cormack 2013-06-18 08:41:52 UTC
Schema is now out of date, however I do have no issue with DBIx::Class and speed, I would like to see it used first in one or 2 new modules, so we perhaps don't need to do a full schema replacement to start with.
Comment 55 Magnus Enger 2013-06-18 08:55:37 UTC
I think the digital signs in bug 8628 would be good candidates for testing DBIC, and for them speed would not really be an issue either. But I'd need some kind of "guarantee" that they were not rejected *because* they use DBIC... :-)
Comment 56 Chris Cormack 2013-06-18 09:00:15 UTC
(In reply to comment #55)
> I think the digital signs in bug 8628 would be good candidates for testing
> DBIC, and for them speed would not really be an issue either. But I'd need
> some kind of "guarantee" that they were not rejected *because* they use
> DBIC... :-)

I'll leave that for Galen :)
Comment 57 Paul Poulain 2013-06-18 09:56:42 UTC
(In reply to comment #54)
> Schema is now out of date, however I do have no issue with DBIx::Class and
> speed, I would like to see it used first in one or 2 new modules, so we
> perhaps don't need to do a full schema replacement to start with.

That's what my bug 8309 proposal intended to do. The schema was a complete one (which is easy to fix), and it included some code to see how we could implement it in some modules.
The http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10477 attachment update admin/categories.pl to use DBIx::Class

( What about having a workshop about that during KohaCon13 hackfest ? )
Comment 58 Kyle M Hall 2013-06-18 17:15:34 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2013-06-18 17:18:31 UTC
Updating the schema files is trivial with this patch set ( misc/devel/update_dbix_class_files.pl ). I would say once this is pushed, future patches that modify the database should include a schema file updates.

I currently have two new developments I am planning that will use DBIx::Class with.
Comment 60 Galen Charlton 2013-06-18 20:32:01 UTC
(In reply to comment #56)
> (In reply to comment #55)
> > I think the digital signs in bug 8628 would be good candidates for testing
> > DBIC, and for them speed would not really be an issue either. But I'd need
> > some kind of "guarantee" that they were not rejected *because* they use
> > DBIC... :-)
> 
> I'll leave that for Galen :)

Chris and I discussed this bug today and he's agreed to update the patch series for master; I intend to push DBIC support unless some showstopper comes up during QA and testing.

Consequently, I think you can safely proceed with further work on bug 8628 under the assumption that DBIC support will be present at some point in the forseeable future.
Comment 61 Chris Cormack 2013-06-18 22:19:49 UTC
Created attachment 19137 [details] [review]
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context

Also added dependencies in required files.
[Update]: changed dependencies fron dbic to dbix
          moved generation shell script to a perl script wtih arguments in misc/devel
To Test:

Install patch
Make sure you can still connect to Koha
you may optionally run the test script in comment 12 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8798#c12
If you run this file you should get a DBIx dump of the borrowers table
Done!

Ammeded Dep version

Signed-off-by: wajasu <matted-34813@mypacks.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 62 Chris Cormack 2013-06-18 22:20:06 UTC
Created attachment 19138 [details] [review]
Bug 8798 - Add the use of DBIx::Class - Rename schema class updater

updateDatabase.pl is a bit too close to updatedatabase.pl in installer
and may cause some confusion. I would suggest update_dbix_class_files.pl
as a unambiguous and descriptive name for this file.

Signed-off-by: wajasu <matted-34813@mypacks.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 63 Chris Cormack 2013-06-18 22:20:25 UTC
Created attachment 19139 [details] [review]
Bug8798 Updated to remove tabs

Signed-off-by: wajasu <matted-34813@mypacks.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 64 Chris Cormack 2013-06-18 22:21:48 UTC
Created attachment 19140 [details] [review]
Bug 8798 - Add the use of DBIx::Class - Followup - Update schema files

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 65 Chris Cormack 2013-06-18 22:22:19 UTC
Created attachment 19141 [details] [review]
Bug 8798 - Moving code to Koha::Database and adding tests

- Fixing a bug .. ping does not exist we need to use connected
Comment 66 Paul Poulain 2013-06-20 14:47:41 UTC
(In reply to comment #60)
> Chris and I discussed this bug today and he's agreed to update the patch
> series for master; I intend to push DBIC support unless some showstopper
> comes up during QA and testing.
Jonathan & I had a short discussion about this patch & DBIC: we have the same question: what's the next step once this patch is pushed ? We need to have clear directions, in order to coordinate the effort & do as much progress as possible in DBIC.

PS: from a QA point of view, am I right if I say that those patches need to be merged in one patch, otherwise they're quite hard to understand ?
the schema is made, then updated. we put something in C4/Context.pm, then remove it to create Koha/Database.pm
Comment 67 Kyle M Hall 2013-06-21 11:53:03 UTC
> Jonathan & I had a short discussion about this patch & DBIC: we have the
> same question: what's the next step once this patch is pushed ? We need to
> have clear directions, in order to coordinate the effort & do as much
> progress as possible in DBIC.

This a good question! I've already submitted a patch that uses DBIC, Bug 10493. It's interesting because I started writing using the standard C4 and Koha modules, but I was extremely surprised to find how much easier and quicker the development went once I switched to DBIC!

I think we need to push as much logic into the DBIC classes as we can ( only the logic which sensibly should be there or course ). For example, I'm sure Koha::Schema::Result::Borrower is ripe for additional logic. C4::Members::IsMemberBlocked() could easily be replaced with $borrower->IsBlocked(). 
 
> PS: from a QA point of view, am I right if I say that those patches need to
> be merged in one patch, otherwise they're quite hard to understand ?
> the schema is made, then updated. we put something in C4/Context.pm, then
> remove it to create Koha/Database.pm

The only issue I see is we have 3 authors here. Elliot is no longer active in the community so I cannot speak for him, though I'm sure we can contact him about it. If a merge before push is necessary, I'd say he should retain the authorship line, considering this was originally his patch.

On the other hand, a squashed patch could be uploaded for QA purposes, and the individual patches pushed afterward. I think that should provide the QA clarity while retaining individual patch authorship.
Comment 68 Paul Poulain 2013-06-21 12:00:50 UTC
(In reply to comment #67)
> > Jonathan & I had a short discussion about this patch & DBIC: we have the
> > same question: what's the next step once this patch is pushed ? We need to
> > have clear directions, in order to coordinate the effort & do as much
> > progress as possible in DBIC.
> 
> This a good question! I've already submitted a patch that uses DBIC, Bug
> 10493. It's interesting because I started writing using the standard C4 and
> Koha modules, but I was extremely surprised to find how much easier and
> quicker the development went once I switched to DBIC!
Have you seen my comment 57 and the other patch I made ? It also includes rewrite for some admin/ scripts.
I agree that it's *much* easier ;-)
(feel free to rebase, and I've no problem with loosing my authorship on those patches, if it can help things to go faster ;-) )

> I think we need to push as much logic into the DBIC classes as we can ( only
> the logic which sensibly should be there or course ). For example, I'm sure
> Koha::Schema::Result::Borrower is ripe for additional logic.
> C4::Members::IsMemberBlocked() could easily be replaced with
> $borrower->IsBlocked(). 
Agreed. OTOH, we must be carefull with the methods we include : not too much, not too few. With the 3 level Business / DataObject / DB, I think things will be easy to manage : as many Business as we need, as small as possible, linked/linking to DO then DB.

> The only issue I see is we have 3 authors here. Elliot is no longer active
> in the community so I cannot speak for him, though I'm sure we can contact
> him about it. If a merge before push is necessary, I'd say he should retain
> the authorship line, considering this was originally his patch.
> 
> On the other hand, a squashed patch could be uploaded for QA purposes, and
> the individual patches pushed afterward. I think that should provide the QA
> clarity while retaining individual patch authorship.
I prefer the 2nd option for copyright purposes, but the 1st one for simplicity & readability.
Comment 69 Galen Charlton 2013-06-21 14:40:26 UTC
(In reply to comment #68)
> > On the other hand, a squashed patch could be uploaded for QA purposes, and
> > the individual patches pushed afterward. I think that should provide the QA
> > clarity while retaining individual patch authorship.
> I prefer the 2nd option for copyright purposes, but the 1st one for
> simplicity & readability.

I think the attribution issue can be addressed when squashing by adding Author: tags to the commit message that list contributor's whose attribute would otherwise be taken away by the squash.
Comment 70 Galen Charlton 2013-06-21 14:55:33 UTC
(In reply to comment #66)
> (In reply to comment #60)
> > Chris and I discussed this bug today and he's agreed to update the patch
> > series for master; I intend to push DBIC support unless some showstopper
> > comes up during QA and testing.
> Jonathan & I had a short discussion about this patch & DBIC: we have the
> same question: what's the next step once this patch is pushed ? We need to
> have clear directions, in order to coordinate the effort & do as much
> progress as possible in DBIC.

What I propose is a multi-part process:

[1] Once DBIC support is pushed, along with at least one other patch that uses it, we promote the existence of it vigorous, with the aim of encouraging folks to use it for new functionality even *before* KohaCon.  I agree that seminars there would be a good idea, too.

I think it would be a reasonable goal to get to the point where, once the cycle for 3.16 starts, DBIC is required for any new code that interacts with the database.  That can't be a requirement for 3.14, of course, but we can certainly encourage it.

[2] I agree that we should make a concerted effort to switch existing code over to DBIC, but with a *very* strong caveat: I am disinclined to push patches that refactor code to use DBIC that are not backed up by unit tests first.  There will be some trickiness in writing such tests, of course, since switching functionality to DBIC also means restructuring the API, but I have very little tolerance for user-visible regressions in the name of architectural improvements, no matter how important.

However, one question that has to be answered first: who has time to commit to systematically refactoring existing code?  An answer to that would inform future planning.
Comment 71 Paul Poulain 2013-06-21 15:11:43 UTC
(In reply to comment #70)
> What I propose is a multi-part process:
> 
> [1] Once DBIC support is pushed, along with at least one other patch that
> uses it, we promote the existence of it vigorous, with the aim of
> encouraging folks to use it for new functionality even *before* KohaCon.  I
> agree that seminars there would be a good idea, too.
+1
> I think it would be a reasonable goal to get to the point where, once the
> cycle for 3.16 starts, DBIC is required for any new code that interacts with
> the database.  That can't be a requirement for 3.14, of course, but we can
> certainly encourage it.
+1, even if the big deal here will be to mix DBIC & C4/ code: I want to add a feature that could use C4/Acquisition.pm::whatever. Should I write a Koha/BusinessLogic::whatever_for_DBIC anyway ? I don't see an clear answer to this question. If I answer yes, then it mean there are 2 "whatever" functions, that will be a pain to maintain.

> [2] I agree that we should make a concerted effort to switch existing code
> over to DBIC, but with a *very* strong caveat: I am disinclined to push
> patches that refactor code to use DBIC that are not backed up by unit tests
> first.  There will be some trickiness in writing such tests, of course,
> since switching functionality to DBIC also means restructuring the API, but
> I have very little tolerance for user-visible regressions in the name of
> architectural improvements, no matter how important.
in my tests on bug 8309, I started with some basic admin/ scripts. I think it's a good way to start, because there is only a few dependencies, and a lot of things that are not in any C4/ file already.

> However, one question that has to be answered first: who has time to commit
> to systematically refactoring existing code?  An answer to that would inform
> future planning.
+1
Next week, there's the yearly french kohacon, I plan to speak openly of this problem: we need time, and, as everyone knows, time is money (I need to pay BibLibre staff you know... what a pity :D ).
I think we should also speak of this during KohaCon13
Comment 72 Kyle M Hall 2013-06-21 15:14:07 UTC
> [1] Once DBIC support is pushed, along with at least one other patch that
> uses it, we promote the existence of it vigorous, with the aim of
> encouraging folks to use it for new functionality even *before* KohaCon.  I
> agree that seminars there would be a good idea, too.

That sounds great to me!
 
> I think it would be a reasonable goal to get to the point where, once the
> cycle for 3.16 starts, DBIC is required for any new code that interacts with
> the database.  That can't be a requirement for 3.14, of course, but we can
> certainly encourage it.

I for one, will be writing all future patches the involve new scripts and subs with DBIC from this point on.

> [2] I agree that we should make a concerted effort to switch existing code
> over to DBIC, but with a *very* strong caveat: I am disinclined to push
> patches that refactor code to use DBIC that are not backed up by unit tests
> first.  There will be some trickiness in writing such tests, of course,
> since switching functionality to DBIC also means restructuring the API, but
> I have very little tolerance for user-visible regressions in the name of
> architectural improvements, no matter how important.

I agree with unit testable-ness for functions, but what about scripts? One of the great reasons to have DBIC is we can just use find() or single() to grab a borrower in the script, rather than calling C4::Members::GetMember. Script mods should of course have a full and thorough test plan, but I can't imagine them being unit testable, right?

> However, one question that has to be answered first: who has time to commit
> to systematically refactoring existing code?  An answer to that would inform
> future planning.

I for one will absolutely commit to refactoring existing code! For smaller modules I could probably submit one a week or more ( considering smaller modules have fewer subs and often fewer callers ). Larger modules with subs called often will probably need to be refactored on a sub by sub basis.
Comment 73 Galen Charlton 2013-06-21 15:20:09 UTC
Paste of an IRC conversation with the maintainer of DBIX::Class today:

<riba> I wanted to comment on http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8798 but didn't find a way without creating yet another account
<huginn> Bug 8798: enhancement, P3, ---, elliott, Signed Off , Add the use of DBIx::Class
<-> riba is now known as Guest688
<-> Guest688 is now known as ribasushi
<ribasushi> wtf..
<ribasushi> anyway - is any of the bug participants around?
<jcamins> ribasushi: I think gmcharlt, khall, and paul_p have been discussing that recently.
<paul_p> ribasushi yes, I'm here
<ribasushi> hi
<-- lds (~Thunderbi@giono.biblibre.com) has quit (Quit: lds)
<paul_p> hi ribasushi
<gmcharlt> hi ribasushi
<ribasushi> paul_p: I am the current-long-standing DBIC maint
<ribasushi> I am excited you guys are migrating DBIC wholesale - I need more such success stories to brag about ;D
<ribasushi> I looked over some of the diffs and there's one thing that jumped at me as very wrong
--> lds (~Thunderbi@giono.biblibre.com) has joined #koha
<paul_p> ribasushi well, we've got the decision, some nice hackers, what we lack is time (or money, but that's almost the same thing : I could dedicate 2 FTE to this task if I had enough money...)
<paul_p> ribasushi which mistake ?
<ribasushi> you have set (in many places) ...
<ribasushi> I am looking for it, lost the diff...
<-- chris_n (~Chris@184.7.21.42) has quit (Quit: Coyote finally caught me)
<ribasushi> $ENV{'DBIC_DONT_VALIDATE_RELS'} <- you set this all over the place
<ribasushi> it was introduced as a backcompat measure for folks with broken preexisting schema definitions
<ribasushi> in *new code* this is a sign of a latent data eater
--> Callender (~quassel@50-79-238-214-static.hfc.comcastbusiness.net) has joined #koha
<ribasushi> you need to resolve the cause of this (whatever it may actually be) before "going public" with a call for conversions
--> chris_n (~Chris@184.7.21.42) has joined #koha
*** Mode #koha +o chris_n by ChanServ
<gmcharlt> ribasushi: we were figuring that corrections to the schema were needed anyway
<gmcharlt> but could you expand on what you mean by "latent data eater"?
<ribasushi> the presence of this warning implies an incorrectly set metadata on the source in question
<ribasushi> most likely the "direction" of the relationship is reversed
<-> Jesse_phone is now known as JesseM
<ribasushi> the higher the DBIC version - the more I rely on the correctness of such metadata to make informed decisions on the SQL I generate
<ribasushi> for instance an incorrect relationship direction may end up in a cascaded delete going in a way that you never anticipated
<ribasushi> or an entire JOIN being dropped away because DBIC can "prove" (based on the erroneous metadata) that it does not need it
<gmcharlt> gah
<gmcharlt> thanks for the warning
<ribasushi> because this stuff happens deep in the guts the consequences may compound to something I can't currently think of
<paul_p> ribasushi = am I right if I say "adding this will hide any error in your schema, and that's a bad idea, better fixing the schema" ?
<ribasushi> on the other hand - I *never* rely on metadata blindly, unless I can provide a reasonable (and timely) warning way in advance "hey your schema metadata makes no sense!"
<ribasushi> gmcharlt: so I am not taking the risk lightly, but there is only so much I can do ;)
<ribasushi> paul_p: "any error" is a tad strong, "a number of errors" would be more correct
<ribasushi> paul_p: also s/better fixing/we have to fix/
<gmcharlt> ribasushi: well, we'll take that out of the patch and exercise it more
<ribasushi> I am sadly not much around IRC lately (life happens etc)
<ribasushi> but I am always available with a reasonable turnaround at ribasushi@cpan.org
--> pianohacker (~feoh3@vps.lanfort.org) has joined #koha
<ribasushi> so feel free to contact me if more issues arise
<ribasushi> gmcharlt: ^^
<ribasushi> paul_p: ^^
<gmcharlt> great -- and thanks for taking the time to look it over
<paul_p> ribasushi we'll probably ask for some hints later ;-)
<gmcharlt> ribasushi: do you have any objections to my pasting this conversation into the bug report? (the channel is logged anyway, to be clear)
<ribasushi> gmcharlt: not at all
<-- lds (~Thunderbi@giono.biblibre.com) has quit (Quit: lds)
<gmcharlt> thanks
<ribasushi> gmcharlt: I'd also grp for other ENV overrides
<ribasushi> in case I missed something (it's a lot of code ;)
<ribasushi> *grep
Comment 74 Galen Charlton 2013-06-21 15:48:24 UTC
(In reply to comment #71)
> (In reply to comment #70)
> > I think it would be a reasonable goal to get to the point where, once the
> > cycle for 3.16 starts, DBIC is required for any new code that interacts with
> > the database.  That can't be a requirement for 3.14, of course, but we can
> > certainly encourage it.
> +1, even if the big deal here will be to mix DBIC & C4/ code: I want to add
> a feature that could use C4/Acquisition.pm::whatever. Should I write a
> Koha/BusinessLogic::whatever_for_DBIC anyway ? I don't see an clear answer
> to this question. If I answer yes, then it mean there are 2 "whatever"
> functions, that will be a pain to maintain.

My preference is that that every effort should be made when introducing a DBIC-based method to remove the C4 function(s) that it supersedes; having both a Koha::* method and a C4::* method for the same thing only invites trouble in the form of divergent business logic.  Developing new features incrementally can help with that.

Of course, I can imagine circumstances where following the recommendation above would require refactoring an unwieldy amount of code.  In cases like that, I will *consider* patches that result in Koha/C4 duplication provided that the C4 function is clearly marked as deprecated.  If such a patch gets pushed, I'd say that no new code that uses the C4 function would subsequently be allowed.

Since all of this refactoring we're facing means that we're essentially building a whole new API, I *strongly* encourage folks to discuss their proposals for refactoring particular methods on koha-devel first.
Comment 75 Jared Camins-Esakov 2013-06-21 15:53:45 UTC
(In reply to comment #74)
> My preference is that that every effort should be made when introducing a
> DBIC-based method to remove the C4 function(s) that it supersedes; having
> both a Koha::* method and a C4::* method for the same thing only invites
> trouble in the form of divergent business logic.  Developing new features
> incrementally can help with that.

I have no irons in this fire, but if the C4 functions can be retained as transitional shims rather than simply deleting them, that might reduce the amount of code that required refactoring, and maintain the usefulness of existing unit tests.
Comment 76 Galen Charlton 2013-06-21 15:55:04 UTC
(In reply to comment #72)
> I agree with unit testable-ness for functions, but what about scripts? One
> of the great reasons to have DBIC is we can just use find() or single() to
> grab a borrower in the script, rather than calling C4::Members::GetMember.
> Script mods should of course have a full and thorough test plan, but I can't
> imagine them being unit testable, right?

That depends on how one writes them.  If we think of a "script" as most of the time being a wrapper that gathers command-line parameters, then calls one or more functions or methods that exist in the Koha namespace, those functions can readily be included in the Test::More-based tests.

In other words, we should encourage writing scripts in such a way that unit tests of their core functionality can be easily written.
Comment 77 Galen Charlton 2013-06-21 15:58:34 UTC
(In reply to comment #75)
> (In reply to comment #74)
> > My preference is that that every effort should be made when introducing a
> > DBIC-based method to remove the C4 function(s) that it supersedes; having
> > both a Koha::* method and a C4::* method for the same thing only invites
> > trouble in the form of divergent business logic.  Developing new features
> > incrementally can help with that.
> 
> I have no irons in this fire, but if the C4 functions can be retained as
> transitional shims rather than simply deleting them, that might reduce the
> amount of code that required refactoring, and maintain the usefulness of
> existing unit tests.

That's a good point.  Certainly one could envision a C4 method being rewritten to invoke a bunch of DBIC-based code, without the unit test changing at all.  But the emphasis would be on such things being *transitional*.
Comment 78 Kyle M Hall 2013-06-21 16:06:23 UTC
I cannot agree more with this assertion. Koha scripts have often been too 'smart' for their own good ; )

Over time this has definitely improved with more and more logic getting pushed into the Modules. I'd say this is where most bugs come from. The bug in the generation of fines in fines.pl vs returns.pl comes to mind. I think a long term goal is to have each system preference checked only once in the code base for any given behavior. That way these types of bugs won't occur. But now I'm getting all pie-in-the-sky and going off topic at the same time!

To summarize, in our ideal Koha, scripts just get a bunch of stuff and pass it to the template, without doing much else.

> That depends on how one writes them.  If we think of a "script" as most of
> the time being a wrapper that gathers command-line parameters, then calls
> one or more functions or methods that exist in the Koha namespace, those
> functions can readily be included in the Test::More-based tests.
> 
> In other words, we should encourage writing scripts in such a way that unit
> tests of their core functionality can be easily written.
Comment 79 Paul Poulain 2013-06-21 16:32:12 UTC
(In reply to comment #78)
> I cannot agree more with this assertion. Koha scripts have often been too
> 'smart' for their own good ; )
> 
> Over time this has definitely improved with more and more logic getting
> pushed into the Modules. I'd say this is where most bugs come from. The bug
> in the generation of fines in fines.pl vs returns.pl comes to mind. I think
> a long term goal is to have each system preference checked only once in the
> code base for any given behavior. That way these types of bugs won't occur.
> But now I'm getting all pie-in-the-sky and going off topic at the same time!
I concur !
And I would add that the 2 level only logic we had (.pl / C4/.pm) was another source of bugs : any logic must sometimes/often be checked in different context, and it's sometimes checked differently.

> To summarize, in our ideal Koha, scripts just get a bunch of stuff and pass
> it to the template, without doing much else.
agreed !

> > In other words, we should encourage writing scripts in such a way that unit
> > tests of their core functionality can be easily written.
Test::www::Mechanize can also help us testing scripts in web-environment as well. It's sometimes tricky to be sure of the results, but, at least, we can be sure it returns a 200 ;-)
Comment 80 Galen Charlton 2013-06-21 16:36:03 UTC
(In reply to comment #79)
> > > tests of their core functionality can be easily written.
> Test::www::Mechanize can also help us testing scripts in web-environment as
> well. It's sometimes tricky to be sure of the results, but, at least, we can
> be sure it returns a 200 ;-)

Better than returning 418! [1] ;)

[1] http://tools.ietf.org/html/rfc2324
Comment 81 Paul Poulain 2013-06-26 19:00:11 UTC
Created attachment 19248 [details] [review]
Added base class files for all tables in koha using the DBIx::Class::Schema::Loader Added a (very basic) test file for C4::Context

Also added dependencies in required files.
[Update]: changed dependencies fron dbic to dbix
          moved generation shell script to a perl script wtih arguments in misc/devel
To Test:

Install patch
Make sure you can still connect to Koha
you may optionally run the test script in comment 12 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8798#c12
If you run this file you should get a DBIx dump of the borrowers table
Done!

Ammeded Dep version

Signed-off-by: wajasu <matted-34813@mypacks.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Bug 8798 - Add the use of DBIx::Class - Rename schema class updater

updateDatabase.pl is a bit too close to updatedatabase.pl in installer
and may cause some confusion. I would suggest update_dbix_class_files.pl
as a unambiguous and descriptive name for this file.

Signed-off-by: wajasu <matted-34813@mypacks.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Bug8798 Updated to remove tabs

Signed-off-by: wajasu <matted-34813@mypacks.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Bug 8798 - Add the use of DBIx::Class - Followup - Update schema files

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Bug 8798 - Moving code to Koha::Database and adding tests

- Fixing a bug .. ping does not exist we need to use connected
Comment 82 Paul Poulain 2013-06-26 19:00:25 UTC Comment hidden (obsolete)
Comment 83 Paul Poulain 2013-06-26 19:00:35 UTC Comment hidden (obsolete)
Comment 84 Paul Poulain 2013-06-26 19:01:28 UTC Comment hidden (obsolete)
Comment 85 Paul Poulain 2013-06-26 19:07:39 UTC
OK, I gave this patch a try, because DBIx::Class is where we want to go in the long term, so it's worth the effort
(plus, I'm stuck in a train that is 2 hours late, soon 3 hours, so I will have 9 hours in a train...)

QA comments :
 * I've squashed the 4 previous patches, it's much easy to read the code, the authorship is still here
 * I think the  "C4/Context.pm   |    1 - "should be removed, it's just removing an empty line
 * unless i'm wrong, the  "t/Context.t  |   10 +" is not related to DBIx::Class. It's a good idea to have it, but please put it in another patch

But's theres more: I wrote a patch to implement a 1st version of Branch.pm (BusinessLogic, DataObject, DB), then made 2 other patches to see how it behaves.
It's nice & handy.
BUT : there's some diacritics in my database, and they're not properly handled. I thought I had found the solution, but got no success.

This problem must be fixed before the initial patch is pushed.

HTH
Comment 86 Galen Charlton 2013-06-26 23:24:07 UTC
I'm not seeing UTF8-releated breakage -- try running this program after applying the squashed patch.  The output (not counting a DBIx::Class::Carp about the nullability of borrowers.cardnumber that should be addressed at some point) is correct:

Institut Protestant de Théologie

#!/usr/bin/perl                                                                                                                    
                                                                                                           
use Modern::Perl;
use Koha::Database;

my $database = Koha::Database->new();
my $schema = $database->schema();

my @branches = $schema->resultset('Branch')->search({ branchcode => 'IPT'})->all();

binmode(STDOUT, ':encoding(UTF-8)');
print $branches[0]->branchname();
print "\n";
Comment 87 Galen Charlton 2013-06-26 23:58:40 UTC
I'm setting this bug back to 'In discussion', as I believe that the Unicode issue that Paul was running into is a red herring.

I have significant reservations about the the POC patches proposed earlier today.  In particular they assume that the namespace RFC has been agreed to -- and at this point, I do not believe that we actually have consensus on it.

More immediately, I have reservations about the Koha::DB layer.  DBIx::Class::Schema::Loader already provides pre-built data acccess classes -- which is the point of using it in the first place.  The Koha::DB::Branch class as proposed would add yet another file to edit whenever a new column is added to a table.  Rather than doing that, I think we should rely on misc/devel/update_dbix_class_files.pl, and correct the schema as needed so that we can avoid ever doing manual updates to the files under Koha/Schema.

I think we should focus on getting DBIx::Class::Schema::Loader support ready, including sussing out any necessary schema updates and dealing with the DBIx::Class maintainer's recommendation to not use DBIC_DONT_VALIDATE_RELS.  I think for the moment code that exercises DBIx::Class can rely on $schema->resultset('Table'), although if people have a real problem with the length of that construct, I think some AUTOLOAD magic would be preferable to manually maintaining an extra layer of DB access classes.

I think that bug 8309 (and the mailing list) is a better place for the discussion of the proposed namespace scheme.  To help keep the discussion in this bug focused, I've obsoleted all of the patches except for the squash (and thanks for doing that, Paul!)
Comment 88 Paul Poulain 2013-06-27 07:52:32 UTC
(In reply to Galen Charlton from comment #86)
> I'm not seeing UTF8-releated breakage -- try running this program after
> applying the squashed patch.  The output (not counting a DBIx::Class::Carp
> about the nullability of borrowers.cardnumber that should be addressed at
> some point) is correct:
> 
> Institut Protestant de Théologie
I tried your small program, and confirm it works. So where does the problem come from... If anyone has an idea...
I still think that we should have the mysql_enable_utf8 flag set, as the documentation suggests that.
Comment 89 Paul Poulain 2013-06-27 07:59:50 UTC
(In reply to Galen Charlton from comment #87)
> I'm setting this bug back to 'In discussion', as I believe that the Unicode
> issue that Paul was running into is a red herring.
> 
> I have significant reservations about the the POC patches proposed earlier
> today.  In particular they assume that the namespace RFC has been agreed to
> -- and at this point, I do not believe that we actually have consensus on it.
> 
> More immediately, I have reservations about the Koha::DB layer. 
> DBIx::Class::Schema::Loader already provides pre-built data acccess classes
> -- which is the point of using it in the first place.  The Koha::DB::Branch
> class as proposed would add yet another file to edit whenever a new column
> is added to a table.  Rather than doing that, I think we should rely on
> misc/devel/update_dbix_class_files.pl, and correct the schema as needed so
> that we can avoid ever doing manual updates to the files under Koha/Schema.
We spoke of it already, at KohaCon12 In Edinburgh, and on koha-devel. See http://wiki.koha-community.org/wiki/Koha_Namespace_RFC

However, I agree it's worth taking time to discuss it, because a lot of things will depend on the decision we take here.

> I think we should focus on getting DBIx::Class::Schema::Loader support
> ready, including sussing out any necessary schema updates and dealing with
> the DBIx::Class maintainer's recommendation to not use
> DBIC_DONT_VALIDATE_RELS.  I think for the moment code that exercises
> DBIx::Class can rely on $schema->resultset('Table'), although if people have
> a real problem with the length of that construct, I think some AUTOLOAD
> magic would be preferable to manually maintaining an extra layer of DB
> access classes.
what do you mean by "length of that construct" ? it's slow (length=duration) ? verbose ? something else ?

I'll have a look at any proposal & make counter-proposals or additions, until we reach the optimal way to do it.
Chris, you're probably more experienced than I'm, so, your help is warmly welcomed ;-)

> I think that bug 8309 (and the mailing list) is a better place for the
> discussion of the proposed namespace scheme.  To help keep the discussion in
> this bug focused, I've obsoleted all of the patches except for the squash
> (and thanks for doing that, Paul!)

No problem, I was not expecting those patches to be pushed though, they were more a POC than real code.
(I think those patches are still usefull to address the encoding problem)
Comment 90 Galen Charlton 2013-06-27 11:41:06 UTC
(In reply to Paul Poulain from comment #89)
> > I think we should focus on getting DBIx::Class::Schema::Loader support
> > ready, including sussing out any necessary schema updates and dealing with
> > the DBIx::Class maintainer's recommendation to not use
> > DBIC_DONT_VALIDATE_RELS.  I think for the moment code that exercises
> > DBIx::Class can rely on $schema->resultset('Table'), although if people have
> > a real problem with the length of that construct, I think some AUTOLOAD
> > magic would be preferable to manually maintaining an extra layer of DB
> > access classes.
> what do you mean by "length of that construct" ? it's slow (length=duration)
> ? verbose ? something else ?

I mean verbosity, i.e., the textual length.
Comment 91 Magnus Enger 2013-06-28 21:28:43 UTC
(In reply to Paul Poulain from comment #89)
> We spoke of it already, at KohaCon12 In Edinburgh, and on koha-devel. See
> http://wiki.koha-community.org/wiki/Koha_Namespace_RFC
> 
> However, I agree it's worth taking time to discuss it, because a lot of
> things will depend on the decision we take here.

How do we move this forward? Are there competing proposals? Should it be a topic for an IRC meeting?
Comment 92 Galen Charlton 2013-07-01 02:22:21 UTC
(In reply to Magnus Enger from comment #91)
> (In reply to Paul Poulain from comment #89)
> > We spoke of it already, at KohaCon12 In Edinburgh, and on koha-devel. See
> > http://wiki.koha-community.org/wiki/Koha_Namespace_RFC
> > 
> > However, I agree it's worth taking time to discuss it, because a lot of
> > things will depend on the decision we take here.
> 
> How do we move this forward? Are there competing proposals? Should it be a
> topic for an IRC meeting?

The namespace question is not a blocker for DBIx::Class support -- the main blocker at the moment is addressing the concern of the DBIC maintainer about the current patch's use of DBIC_DONT_VALIDATE_RELS.  I will be looking at this over the next couple weeks.

As far as the namespace questions are concerned, because of the potential complexity, in my view an IRC meeting is not well-suited to discussing it; as I previously stated, bug 8309 and/or koha-devel are more suitable.
Comment 93 Paul Poulain 2013-07-05 08:48:03 UTC
(In reply to Galen Charlton from comment #92)
> As far as the namespace questions are concerned, because of the potential
> complexity, in my view an IRC meeting is not well-suited to discussing it;
agreed
> as I previously stated, bug 8309 and/or koha-devel are more suitable.
as there's also a wiki page, it can also be used for discussion: http://wiki.koha-community.org/wiki/Talk:Koha_Namespace_RFC
Comment 94 Galen Charlton 2013-10-14 21:26:32 UTC
At long last, this is in master.  I've pushed a few follow-ups, but the only one of particular note is one that requires update_dbix_class_files.pl to get its DB connection parameters from the command line rather than the current Koha context, to reduce the risk that a developer or RM accidentally includes test database schema cruft when doing a schema class update. 

Thanks, everybody!
Comment 95 Jonathan Druart 2013-10-15 13:36:21 UTC
Software error on cgi-bin/koha/members/member.pl:
Base class package "DBIx::Class::Schema" is empty.
    (Perhaps you need to 'use' the module which defines that package first,
Comment 96 Volkan Sonmez 2014-01-28 14:28:55 UTC
(In reply to Jonathan Druart from comment #95)
> Software error on cgi-bin/koha/members/member.pl:
> Base class package "DBIx::Class::Schema" is empty.
>     (Perhaps you need to 'use' the module which defines that package first,

This error occurs when DBIx::Class perl module is not installed. Check this perl module. If not installed, install it and try again.