This bug will lay the foundation for using DBIx::Class. It will also include the class files from the automated class generator.
Created attachment 12403 [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
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!
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
Created attachment 12405 [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. To Test: Install patch Make sure you can still connect to Koha Done!
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"; } }
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.
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.
Created attachment 12429 [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 Added test files in the root directory (dummytest.pl) To Test: Install patch Make sure you can still connect to Koha you may optionally run dummytest.pl If you run this file you should get a DBIx dump of the borrowers table Done!
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.
> 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.
> 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?
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"));
Created attachment 12439 [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!
Created attachment 12442 [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! http://bugs.koha-community.org/show_bug.cgi?id=8798 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12443 [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.
Excellent work! I can't wait to use this!
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?
(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.
(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.
(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.
(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
I still get ActionLogs.pm Strange
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
Wajasu perhaps it is a package oddity. Colin what are you testing on? Should this prevent this from being signed off on?
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.
Created attachment 12453 [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!
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.
(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
Created attachment 12477 [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!
Created attachment 12478 [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
Created attachment 12480 [details] [review] SIGNED-OFF-Added-base-class-files-for-all-tables-in
Created attachment 12481 [details] [review] SIGNED-OFF-Bug-8798-Add-the-use-of-DBIx-Class-Rename Test: koha@biblio:~/kohaclone$ prove t/Context.t t/Context.t .. ok All tests successful. Files=1, Tests=1, 1 wallclock secs ( 0.02 usr 0.00 sys + 0.78 cusr 0.10 csys = 0.90 CPU) Result: PASS koha@biblio:~/kohaclone$ ./koha_perl_deps.pl -m -u and didn't see libdbix-class-schema-loader mentioned, as its aligned with official debian squeeze version 7000-1 package. Note: This patch only really gets DBIx into the environment. The functionality of the dbicdump generated classes is not tested here, but this patch enables a more gradual rollout of DBIx functionality. It would be worth noting in a comment, what the exact command line that was used to generate the Koha::Schema. and what version of libdbix-class-schema-loader that was used to generate the source. FYI: Typically one would not check in generated source, but generate it at build time, but DBIx uses a hash of the content in the source to determine if it has been changed. Once someone changes it, the dbicdump won't overwrite it anymore in future runs, unless configured to. Note: Someone may want to address if we would like to apply the DateTime component, to get dates inflated, if that is something worth doing across the board. Also, someone mentioning a desire to use "Versioned" classes, probably for the use of database migrations.
(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
Created attachment 12506 [details] [review] [Follow-up] Updated to remove tabs
Created attachment 12520 [details] [review] [SIGNED-OFF] 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: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12521 [details] [review] [SIGNED-OFF] Updated to remove tabs http://bugs.koha-community.org/show_bug.cgi?id=8798 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12538 [details] [review] 0003-SIGNED-OFF-Bug8798-Updated-to-remove-tabs Regnerated 0003-SIGNED-OFF-Bug8798-Updated-to-remove-tabs to apply. Ran perlcritic on C4/Context.pm
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.
wajasu, have you seen my patches on bug 8309 ? I think it's worth a shared effort
wajasu, ping again !
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 ***
After closely looking at bug 8309 it appears that this is a more closely integrated solution.
re-adding the sign-off status from wajasu
(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
Created attachment 17581 [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 http://bugs.koha-community.org/show_bug.cgi?id=8798 Signed-off-by: wajasu <matted-34813@mypacks.net>
Created attachment 17582 [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>
Created attachment 17583 [details] [review] Bug8798 Updated to remove tabs http://bugs.koha-community.org/show_bug.cgi?id=8798 Signed-off-by: wajasu <matted-34813@mypacks.net>
Created attachment 17584 [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 http://bugs.koha-community.org/show_bug.cgi?id=8798 Signed-off-by: wajasu <matted-34813@mypacks.net>
Created attachment 17585 [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>
Created attachment 17586 [details] [review] Bug8798 Updated to remove tabs http://bugs.koha-community.org/show_bug.cgi?id=8798 Signed-off-by: wajasu <matted-34813@mypacks.net>
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
(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.
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.
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.
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... :-)
(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 :)
(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 ? )
Created attachment 19134 [details] [review] Bug 8798 - Add the use of DBIx::Class - Followup - Update schema files
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.
(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.
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>
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>
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>
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>
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
(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
> 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.
(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.
(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.
(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.
(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
> [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.
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
(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.
(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.
(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.
(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*.
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.
(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 ;-)
(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
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
Created attachment 19249 [details] [review] DBIx::Class Branch.pm implementation This package, and the test implement CRUD for Branches table. Very basic, but proves we can do many things in a few lines !
Created attachment 19250 [details] [review] DBIx::Class tests: replacing GetBranchName by it's DBIx::Class equivalent In this patch, I choose to overload the C4/Branch.pm/GetBranchName by it's DBIx::Class equivalent It's just a POC, no need to commit this patch. the question behind the patch is: should we * REMOVE GetBranchName by replacing all the calls by Koha::BusinessLogic::Branch->read({'branchcode' => $branchcode})->first->branchname; * overload the sub and make it die later I think the 1st will require more effort, but better, while the second requires less effort, but result in much less code cleaning My preference would go to the 1st option.
Created attachment 19251 [details] [review] Testing DBIx::Class, continued I tried to replace the GetBranchesLoop() call in C4/Auth.pm In my database, there is a branchname with a diacritic. Unfortunately, it's not properly handled: "Institut Protestant de Th�ologie" appears instead of "Institut Protestant de Théologie" If I force the page encoding to latin1, it appears correctly. Investigating DBIx::Class documentation, I find a Using Unicode chapter in DBIx::Class::Manual::Cookbook which says: MySQL supports unicode, and will correctly flag utf8 data from the database if the "mysql_enable_utf8" is set in the connect options. my $schema = My::Schema->connection('dbi:mysql:dbname=test', $user, $pass, { mysql_enable_utf8 => 1} ); When set, a data retrieved from a textual column type (char, varchar, etc) will have the UTF-8 flag turned on if necessary. This enables character semantics on that string. You will also need to ensure that your database / table / column is configured to use UTF8. See Chapter 10 of the mysql manual for details. See DBD::mysql for further details. This patch also adds the + mysql_enable_utf8 => 1, # REQUIRED to handle properly utf8 line, but unfortunately, it does not fix the problem. ideas welcomed...
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
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";
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!)
(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.
(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)
(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.
(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?
(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.
(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
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!
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,
(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.