On Wed, Nov 5, 2008 at 5:42 AM, Paul POULAIN <paul.poulain@free.fr> wrote:
Joe Atzberger a écrit :
Paul --
I think you mean all the SQL should be in C4, and yes I agree with that. C4::Table does not receive any SQL as arguments to select, insert or delete. I.E., it preserves the encapsulation of SQL inside C4/*.
This would speed up development of features because a C4/NewFeature.pm module might not be necessary at all, or could focus on things special to NewFeature besides basic table operations. Once a table is added to the DB, C4::Table can access it, no new code necessary.
And the other code will be more recognizable, using the same kind of $table->select({fieldname=>"value"}) statements. Would be nice to be able to use the same OO code accessing issues and old_issues, or any of the XX and old_XX tables, for example.
let me be more precise : do you agree that $table->select({fieldname => "value"}) is SQL somewhere : fieldname.
It is the table fieldname, yes. But that is not SQL. It would be the same as if you were querying some object unrelated to a DB table. If we change fieldname to newfieldname, with Koha as it's written now,
you'll just have to dig C4 to find all occurrences of fieldname and update them.
If you embeed $table->select... in a .pl, then that will be much more complex to maintain.
Not at all. The current approach that most frequently returns a reference to hashref (or reference to array of hashref) ALSO expects you to know the fieldnames in each script retrieving the data. This same argument style is already present in functions like ModMember. Perhaps it's useful to approach the question in reverse. Say you have one table and you to write a C4::Module to allow me to query that table. If all we ever needed was GetRowByPrimaryKey, it wouldn't be an issue. But we know we want to be able to search against any field. How would you do it? You could have: use C4::Module; my $row_a = GetRowByField1($field1); my $row_b = GetRowByField2($field2); my $row_c = GetRowByField3($field3); # ... and so on ... But that is obviously the least efficient and most duplicative approach. If you have n fields, you would need n different functions. It also fails if we want to specify matching against two fields. It should be no surprise that we don't solve that problem by making a whole bunch of specialized combo functions like: my $row_d = GetRowByField1xField3($field1, $field3); That would only make matters worse, requiring additional functions for every pairing of fields (equalling the sum 1 + 2 + 3 ... + n-1). Very silly indeed, since then we might want to specify 3, 4 or all n fields. So we do one big function: my $row_e = GetRow($field1, $field2, $field3 ... ); This is representative of the approach taken by parts of Koha. Although in most cases Koha doesn't implement it, we would need to have n argument slots for complete functionality. We then have the problem of expressing when we want the function to ignore an argument, and when we want to filter on a value like zero. And it starts to get ridiculous when you see calls like: GetRow(undef, undef, undef, 3, undef, 0, ""); Sometimes the argument order matches the order of fields in the table, and sometimes it doesn't. I think having to remember the numerical position of ALL n arguments is far worse than having to remember the names of just the fields you care about. In particular the problem of updating some fields in a record, without updating ALL of them brings us to Koha's most advanced argument style: ModRow({id=>$id, field1=>$field1}); # or GetRow({fieldname=>"value"}); But back to our hypothetical, say you were able to write the code to accept a hashref and return the correct rows from the table. It might look a bit like the code in C4::Tags. Paul, you seem to object to using the fieldnames as the keys in the hashref, but nothing else would be more sensible. There's no point to making me use a different label when the purpose really is to match against a specific table field. Certainly it is preferable to the old style of: my $member = GetMember($arg, 'cardnumber'); C4::Table is good enough to carp at me if I supply an unrecognized key. Passing hashref arguments is best practice<http://www.perl.com/pub/a/2006/02/23/advanced_subroutines.html>, afaict <http://safari.oreilly.com/0596001738/perlbp-CHP-16-SECT-4>, and allows clean conversion to OO style, where GetRow actually is more like a constructor: my $rowobj = C4::Module->new({fieldname=>"value"}); So imagining that you finish C4::Module and it works like we want... ok, good! After adding insert, modify and delete functions, you would still have to write similar new code for every new DB table added. C4::Table gives it to you in one place, with error checking and internal caching of structural data, for every existing Koha table and every Koha table that *could* exist, using (in preliminary form) less than 250 lines of code! I think that's about as many lines as we've now used in conversation about it, and I admit I probably should put a lot of this into the module as perldoc to make more apparent my motivation behind it. Hopefully that is more clear now, even if we may disagree about argument style. If you know a preferable alternative to hashref arguments, I would be glad to hear more about it. --Joe