Page 14 of 20

Posted: Tue May 30, 2006 2:19 pm
by santosj
Ambush Commander wrote:You're invited to implement SPL for the DB result set. :-) Just make sure you update the unit tests accordingly.

The method is named next() because... well, I really don't know why. It implies iteration. Sort of. Maybe fetch() is more appropriate.

Actually, empty folders can be added... and since there's really no collisions right now or working copies to break, I request that you do the development in the trunk. (so that I can see the changes, really. :-)
It is done.

Mysql Iterator Class and I also added the test for it.

However it is not as fast as PDO iteration because the PDO implementation is C/C++ and in the PHP distro. Also, normal mysql functions should not be used in PHP 5. Mysqli is desired. However, since you will want to implement a PHP 4 version it is good to have it. However the iteration will only work in PHP 5, the class methods will still work, just can't be used in foreach loop.

I was thinking about implementing a mysqli version and Mysqli has both class and function versions. Mysqli also has better functions for handling iteration, but does not do so natively like SQLite does.

Posted: Tue May 30, 2006 2:29 pm
by Ambush Commander
Okay. You're going to have to enlighten me about database access in PHP5 because I'm still in PHP4 mode. Given a good enough interface, we should be able to switch between mysql, mysqli and mysqlpdo interchangeably.
I also added the test for it.
You added sample code for it: you didn't add a "unit test". ;-) http://www.lastcraft.com/

::in process of cleaning up the code::

By the way, can you set your editor to use four spaces instead of tabs?

Edit - Okay, I've cleaned it up, diff the versions and see what I changed.

Some conventions I'd like to establish right now:

1. 4 spaces. No tabs.
2. A file must have consistent bracing conventions. If the original author put the brace on the same line as the function declaration, keep it that way. Same goes vice versa. However, class definitions MUST have brace on a new line
3. Do not check in code if it breaks unit tests. We're TDDing
4. Do not check in code if it doesn't have unit tests
5. Have autoprops svn:keywords=Id, so that $Id$ gets substituted each time (those with TortoiseSVN can access the autoprops settings by clicking Settings > Subversion Configuration File Edit > add *.php = svn:keywords=Id to the bottom of the file
6. Windows line endings while development is happening. Sorry.

Also, for those of you on Windows systems, there is a nifty little batch script called add_new_class.bat which will do most of the work creating directories/classes/test files for you.

Edit 2 - I think we can tack on SPL as an added feature of the regular MySQL result set. Currently investigating.

Edit 3 - Okay, more comments about the code.

Regarding mysql_fetch_assoc, it seems that it isn't too much of a hit performance-wise, but it IS case sensitive assoc array wise (and not case sensitive in MySQL). With variation with other database systems (others may be case sensitive), I find it much easier just to use numeric indexes. This also forces all column names to be declared in the SQL, which is a GOOD thing.

Regarding your error message handling, we can't use error numbers in good faith and polymorphism, simply put, error numbers in MySQL are different in, say, PostgreSQL. For times when explicitly testing, say, a unique key violation, we'd have to define some way to ask the object, directly, isErrorUniqueKeyViolation(). But otherwise, the error number is slightly useless, and, in quite a few error messages, the number is given in the message!

Edit 4 - The mysql_data_seek() seems to be a bit goofy to me. It doesn't actually do anything, since mysql_fetch_* already advances the inner pointer. How do we get around this?

Posted: Tue May 30, 2006 10:20 pm
by santosj
Ambush Commander wrote:Edit 4 - The mysql_data_seek() seems to be a bit goofy to me. It doesn't actually do anything, since mysql_fetch_* already advances the inner pointer. How do we get around this?
Whoa, some of the stuff I'm going to do, but 4 spaces instead of tab? I could do that yeah, my editor can do that.

The reason I use mysql_data_seek() is because you need to rewind back to the beginning and it adds clearity thoughout what you are doing. It also works nicely with mysql_fetch_*() functions. However, it isn't something that I have thought of so it could optimize the iterator a little bit. If it could remove the two private variables then I'm all for it. However the valid() method would have to be optimized.

Posted: Tue May 30, 2006 11:07 pm
by RobertGonzalez
Ambush Commander wrote:I think it would be helpful if I delegated modules to other people, who would write code for them, then we'd compare notes and integrate it with the rest of the system. Any takers?
I would normally jump at the chance to volunteer for something like this, but I am so overloaded with projects right now that I would certainly let all of the rest of the team down. I am having a hard time keeping up with this thread right now as it is.

Posted: Wed May 31, 2006 4:16 am
by Maugrim_The_Reaper
Forgive me for jumping in late - juggling work and commitments is a pain in the @$$. I'm catching up on the thread...
Also, normal mysql functions should not be used in PHP 5. Mysqli is desired. However, since you will want to implement a PHP 4 version it is good to have it. However the iteration will only work in PHP 5, the class methods will still work, just can't be used in foreach loop.
Normal MySQL can be used. MySQL3 anyone... MySQLi was designed specifically for 4.1.3 or better. I need to read the code, but presumably using MySQLi as the base assumption does not make it difficult to still work with MySQL or PgSQL libs. I think pg_fetch_result() has similar functionality for mysql_data_seek() (as a note-to-self).
I'm not too excited about the DB system, it is a work of art, but I think it is a little bit more complex than it needs to be.
Seems quite simple actually. The abstract class is pretty short. I feel put out without a PostgreSQL option though...;). Maybe I submit that.
Oh! That brings a naming convention issue: should we name interfaces/abstract classes like iAuthorizationManager? Interface/AuthorizationManager? Or just AuthorizationManager is an interface for everything (I kind of like that last idea, and that's how the DB class is working.)
I think interfaces should have the "i" prefix. Abstracts can follow the same pattern as any generic parent class you might use. The "i" prefix is fairly common, but if you have another preference then so long as its consistent would still be fine.
The method is named next() because... well, I really don't know why. It implies iteration. Sort of. Maybe fetch() is more appropriate.
fetch() makes more sense IMO. next() does suggest an iteration over a fetching row data.
By the way, can you set your editor to use four spaces instead of tabs?
Ye gods...;)
6. Windows line endings while development is happening. Sorry.
Hmm..isn't there a svn prop called eol-style you can set to allow Windows/Unix line endings be made consistent regardless of the platform?

After checking: http://svnbook.red-bean.com/nightly/en/ ... .eol-style , should be able to set this up (under Windows) in the TortoiseSVN config file - same as for the svn:keywords property setting described earlier.

*.php = svn:eol-style=LF

Think this will ensure those on Windows still commit files with Unix LF style new lines... Can also set it via the console client under Unix with the propset syntax.

I still need to review this thread - all 13 other pages of posts. Is there a list of required modules? At the very least I can add support for PostgreSQL and MSSQL on the DB side.

Posted: Wed May 31, 2006 8:01 am
by santosj
Do we actually want people to use the DB system? I thought it was just for testing. It could be a put off for other people to be forced to use our DB system when there are a bunch of others out there. I believe that there should be interfaces for which AuthTool's DB interfaces with the rest of the AuthTool as well as allow the other developers use their own DB abstraction classes.

Even so, there should be a 'lite' version that does not include the DB classes and only has the main AuthTools with the interfaces for the database.


EDIT

One of the benefits of working in a group is that you are critique more often, which will improve the overall code. I will work on the iterator and improve it based on your suggestion. I do hope that it will be faster than it is now.

EDIT 2

I updated the Iterator and removed the index and max. It now only uses the mysql_fetch_* functions for moving forward and the valid() checks to see if the fetch functions returned false. It also allows you to specify whether you want to have NUM, ASSOC, or BOTH. Default is ASSOC, but it should be BOTH which is the default for mysql_fetch_array function.

I didn't do a unit test or check for errors, but I'm to add the unit test for the isResource() and setFetch() (which needs a better name).

Posted: Wed May 31, 2006 3:02 pm
by Ambush Commander
Whoa, some of the stuff I'm going to do, but 4 spaces instead of tab? I could do that yeah, my editor can do that.
Ye gods... ;-)
I understand it's a religious issue for some, but it's really the way I work. We can add vim/emacs metadata to the files if necessary.
I would normally jump at the chance to volunteer for something like this, but I am so overloaded with projects right now that I would certainly let all of the rest of the team down. I am having a hard time keeping up with this thread right now as it is.
Forgive me for jumping in late - juggling work and commitments is a pain in the @$$. I'm catching up on the thread...
Contribute when you have time. :-)
The reason I use mysql_data_seek() is because you need to rewind back to the beginning and it adds clearity thoughout what you are doing. It also works nicely with mysql_fetch_*() functions. However, it isn't something that I have thought of so it could optimize the iterator a little bit. If it could remove the two private variables then I'm all for it. However the valid() method would have to be optimized.
mysql_data_seek is essential for rewinding, I agree. The new way makes a bit more sense.
Normal MySQL can be used. MySQL3 anyone... MySQLi was designed specifically for 4.1.3 or better. I need to read the code, but presumably using MySQLi as the base assumption does not make it difficult to still work with MySQL or PgSQL libs. I think pg_fetch_result() has similar functionality for mysql_data_seek() (as a note-to-self).
What's MySQL3 (I get directed to mysqli...)? And I'm under the impression that mysql and mysqli are essentially the same in terms of the functions they provide (though mysqli also provides an OO interface to it)
Seems quite simple actually. The abstract class is pretty short. I feel put out without a PostgreSQL option though...Wink. Maybe I submit that.
That would be great. I specifically made the DB class with PostgreSQL in mind, but I don't have a working installation of the database lying around.
I think interfaces should have the "i" prefix. Abstracts can follow the same pattern as any generic parent class you might use. The "i" prefix is fairly common, but if you have another preference then so long as its consistent would still be fine.
I prefix it is. I'll update the code accordingly.
fetch() makes more sense IMO. next() does suggest an iteration over a fetching row data.
It's fetch() in the SVN now
Hmm..isn't there a svn prop called eol-style you can set to allow Windows/Unix line endings be made consistent regardless of the platform?

After checking: http://svnbook.red-bean.com/nightly/en/ ... .eol-style , should be able to set this up (under Windows) in the TortoiseSVN config file - same as for the svn:keywords property setting described earlier.

*.php = svn:eol-style=LF

Think this will ensure those on Windows still commit files with Unix LF style new lines... Can also set it via the console client under Unix with the propset syntax.
Better yet, we could use svn:eol-style=native. I'll set that autoprop in my SVN installation and update the files in our repository accordingly.
I still need to review this thread - all 13 other pages of posts. Is there a list of required modules? At the very least I can add support for PostgreSQL and MSSQL on the DB side.
Take your time. I'm trying to summarize the entire theoretical discussion, and I'm only at page 6. Support for other DB wrappers would be nice. I'll have to add some docs on how to set optional unit tests with external resources (most of the times, we'll want to use stubs).
Do we actually want people to use the DB system? I thought it was just for testing. It could be a put off for other people to be forced to use our DB system when there are a bunch of others out there. I believe that there should be interfaces for which AuthTool's DB interfaces with the rest of the AuthTool as well as allow the other developers use their own DB abstraction classes.
There is an abstract DB class, if that's what you're looking for. There's no "forcing" to use the database, that's why I kept things as simple as possible, so that adapters would be easy to write.
Even so, there should be a 'lite' version that does not include the DB classes and only has the main AuthTools with the interfaces for the database.
Correct. I've been thinking about this too, and I'm not sure how we would automate the tagging process for "lite" versions.
One of the benefits of working in a group is that you are critique more often, which will improve the overall code. I will work on the iterator and improve it based on your suggestion. I do hope that it will be faster than it is now.
We don't have a benchmark to really say. I can set up a scaffold for that too, if necessary, but it's just DB abstraction.

I'm still not really sold on the foreach($result as $row) idea. If we decide to access rows that way, EVERYBODY should access rows that way, and we phase out the old fetch() method. Iterator, to me, is just an extra feature: while($row = $result->fetch()) still works perfectly fine (and is what most people are used to using).
I updated the Iterator and removed the index and max. It now only uses the mysql_fetch_* functions for moving forward and the valid() checks to see if the fetch functions returned false. It also allows you to specify whether you want to have NUM, ASSOC, or BOTH. Default is ASSOC, but it should be BOTH which is the default for mysql_fetch_array function.
Sounds good. The only drawback to this method is two copies of the row get made before it's returned back. The choice between ASSOC, NUM and BOTH, are, once again, a nice feature to have, but not absolutely necessary.

Edit: - I think it would be good to establish some standards here.
I didn't do a unit test or check for errors, but I'm to add the unit test for the isResource() and setFetch() (which needs a better name).
Have you set up the SimpleTest harness yet? (fortunantely, your update doesn't break any unit tests, but the unit test for MySQLIterator is kind of sparse).

Also, the class seems to logically be an offshoot of the MySQL class, so I might end up moving it.

Edit - Also, is there any point in code like...

Code: Select all

$array;
I mean, it doesn't prevent undefined variable notices for popping up, etc, etc. I would at least set a default value to it (like false or array()) even if it's there for clarity.

Bug report! - I haven't verified it yet, but if we have both associative and numerical keys being returned, then we can't assume that a row with only one column will have a size = 1 (it will have a size = 2 for both)

Okay, cutting down on features - The MySQLIterator class has currently superceded the original Result class in terms of convenience functions. Since our goal isn't to provide a nice DB wrapper, we need to keep things as lean as possible. I propose we cut out some of the extra convenience, esp the one column auto-casting and the choose numeric or associative.

Posted: Thu Jun 01, 2006 6:21 am
by santosj
Yeah, I was owned in the comments. Okay, I understand. I'll strip out some of the components, make a DB iterator abstract class for other database types and work on adding mysqli.

I'm also working on the ACL but it will need to be refactored later on to be able to work with the rest of the AuthTools. What I'm doing is going to need review, the refactor is a given. I just hope that most of it can still be used at a later date.

I just wish you can setup a Trac Site for visual viewing of changes, but you can't with the setup Dreamhost has.

Bug Report

I also found a bug, parent class constructors are not called by default. I think not too long from now we'll need to either get a sourceforge or Mantis or simple Trac site (without Browse Source) up for bug reports.

Iterator Changes

The next() method now only uses the internal array so there is no copying. Also, the options for returning type are gone. I created another class for returning numbered result arrays.

I haven't commited my changes yet.

Posted: Thu Jun 01, 2006 3:49 pm
by Ambush Commander
Yeah, I was owned in the comments.
Just to make things clear, I'm not trying to one-up you. It's just that I hold very high standards of code, which sometimes gets in the way (things just don't turn out satisfactory). It's a learning experience.

However, I will regress and point out that you added a commit without any log message. ;-) Absolutely a no-no.
I'll strip out some of the components, make a DB iterator abstract class for other database types and work on adding mysqli.
This does pose an interesting question: how do we tell the DB class to use the iterator in the first place? And no, a global attribute is probably not acceptable. I'm going to try to merge the two classes together.

Edit - I'm still not sold on the idea of the iterator. Remember: they probably don't want to use our class, so they'd have to write an adapter to their class. The iterator just adds more stuff for them to implement.
I'm also working on the ACL but it will need to be refactored later on to be able to work with the rest of the AuthTools. What I'm doing is going to need review, the refactor is a given. I just hope that most of it can still be used at a later date.
You know, that reminds me of a writer's adage: "Kill your babies." Try to commit it as soon as possible so it can get as many eyes as it can.
I just wish you can setup a Trac Site for visual viewing of changes, but you can't with the setup Dreamhost has.
I think I can. I have shell access, and Dreamhost has python installed. But what is trac, and why is it so good?
I also found a bug, parent class constructors are not called by default.
That's more of a language feature. This is one very compelling argument against inheritance in favor of composition. Be careful with it. Not really a bug though.

Edit 3 - I now see what you're talking about. As long as you don't overload __construct(), the parent constructor will automatically be called. I see no need to redefine it, as the DB class structure is fairly rigid anyway.
I think not too long from now we'll need to either get a sourceforge or Mantis or simple Trac site (without Browse Source) up for bug reports.
If you like, I can set up a Bugzilla. However, it's not very good to have all the eggs in one basket, namely me. Sourceforge is a good, neutral place, and we should be using it.
The next() method now only uses the internal array so there is no copying.
Ack, that's not what I mean. What I meant is that, theoretically speaking, next() shouldn't cause any change of state except the advancing of the pointer, and current() should allocate the memory and return the result. The system we have right now is largely unavoidable, so I don't think it's necessary to add the extra stuff.
I created another class for returning numbered result arrays.
Okay, okay, we'll use associative results. I'm going to kill the numbered result option.
I haven't commited my changes yet.
:?

Anyway... try to stay in-line with the naming conventions where underscores become slashes. Also, it's really really really really important that you start unit testing. The update has broken unit tests again. :-( Edit - I can't see what changed because you renamed classes and made edits (please do only one or the other), so I'm sorry, but I'm forced to rollback.

Edit 2 - I'm slowly munching through your edits. How ever slow require_once may be, it certainly is faster (because it's language level) than manually checking the class's existance.

Edit 3 - I think the reason why I'm having trouble tracking the code change is because you didn't use svn's rename command, instead, you manually cut and pasted, and then deleted the original. Rename CAN traverse directories, for instance, "../MySQLIterator.php" puts the class in the parent directory. Try to use it.

Edit 4 - Okay, so you named your new class AuthTools_MySQLIterator but placed it in DBIterator/MySQLIterator.php. Suppose we were to add __autoload functionality, and we were asked for that class. How would we know it was in the DBIterator directory? The correct name should be AuthTools_DBIterator_MySQLIterator (which is a bit redundant: I'd just rename the file so the name would be AuthTools_DBIterator_MySQL)

Edit 5 - As I dig deeper, I see some very good stuff. Remember: you were reverted on procedural grounds, for not updating the unit tests or giving a log message. Your commit is still accessible at revision 44.

Edit 6 - For those who haven't noticed, there is now a conventions file at docs/conventions.txt. Try to follow these rules, or risk... rollback! :twisted:

Posted: Thu Jun 01, 2006 5:00 pm
by Maugrim_The_Reaper
If you need any help when implementing unit tests give me a shout - it takes a little getting used to. Just checked out the code for a read through.

Posted: Thu Jun 01, 2006 5:21 pm
by santosj
I said to myself, "How would anyone even read the commit message?" but appearently you can access it by other means and I haven't tried the "Log option."

I don't know, I guess I'm trying to reimplement the PDO classes. I again thought about doing the unit test, but choose not too. I'll set one up with the changes.

Let me add some justification for the Iterator, using the while loops are a bane for me.

Code: Select all

while($row = $mysql->fetch())
{
    print_r($row);
}
Or

Code: Select all

foreach($mysql as $row)
{
    print_r($row);
}
Speeds comparsions tell that the foreach loop is a little slower, but for readablity, I like the foreach. Both are supported, however the foreach looping is automatically supported. Meaning you only need to pass the class to the loop instead of the function.

I want to use IteratorAggregate for the regular DBResult classes in case they also want to use it in the foreach loop.

However this might end up going into a while loop vs foreach loop debate/discussion and I don't think it is worth it. There is already support for both methods and I think it should be up to the developer which they are most comfortable with.

However, whenever the AuthTools is backported, the Iterator Classes will have to be stripped out of that version as PHP 4 won't support them. Using IteratorAggregate, you can just remove it for those versions.

SourceForge

I think we should have a little bit more code to show before true bug reporting and submittion to SourceForge is done. I like how it is now with critique, I'm learning a great deal more than I do on my own.

Trac on Dreamhost

http://wiki.dreamhost.com/index.php/Trac

It isn't as easy as Mantis to install and it will take about 1 or 2 hours to about 4 hours.

Posted: Thu Jun 01, 2006 5:31 pm
by Ambush Commander
Okay. Here's the thing. Remember, the developers are US, we're the only ones using the DB code (hopefullly). If we do decide to backport it, every instance where we used it like a foreach() will have to be changed. It's quite a different issue, really, from the standard while versus foreach discussion.
I think we should have a little bit more code to show before true bug reporting and submittion to SourceForge is done. I like how it is now with critique, I'm learning a great deal more than I do on my own.
http://www.joelonsoftware.com/articles/ ... 00043.html

Bug tracking is pretty much a must. Even if we stick them in the source files themselves. Bugzilla is dead useful: I use it for my own, one-developer projects.
I said to myself, "How would anyone even read the commit message?" but appearently you can access it by other means and I haven't tried the "Log option."
It's the log, yup.
http://wiki.dreamhost.com/index.php/Trac

It isn't as easy as Mantis to install and it will take about 1 or 2 hours to about 4 hours.
Oh dear. That does look like a real killer. Not knowing python doesn't help either. We'll see... at the very least, I'll install ViewVC or something.

Posted: Thu Jun 01, 2006 6:15 pm
by santosj
I don't know, I'm really waiting for the rest of the AuthTools before I see any way to majority refactor the DB Abstraction. I'll wait for another person to take a look at it and continue work on the other module. The sooner I get started on that, the sooner I can have it reviewed.

Posted: Thu Jun 01, 2006 6:24 pm
by Ambush Commander
Well, most of the application won't have SQL. We're going to corrale that all in a few gateways or mappers.

And that's a good question.

Active record, Gateway, or Mapper?

Edit - Also, two topics going on recently, very informative!

viewtopic.php?t=49400 - cookie security
viewtopic.php?t=49386 - updating hashes

Posted: Fri Jun 02, 2006 11:36 am
by santosj
I'm sure that someone will provide for the other one or two once everything else is done. I would go for the that would be the easiest one for n00bs and those who are still intermediate level.