Suggestions

1. Please start your class with '<?php' as recommended rather than '<?'.

2. Default values in class and __construct() method. I like the idea of
opening a connection automaticaly, however, I would change defaults
check in __construct method to:

if (strlen($this->db_host) > 0 &&
strlen($this->db_user) > 0 &&
strlen($this->db_pass) > 0) {

note a change in third line from "$this->db_dbname" to "$this->db_pass".
The original code wouldn't allow an automatic connection without
selecting of a database (eg. server with no databases created yet). The
three parameters above are always needed to establish a connection.
In addition I would introduce a new variable (default character set):

private $db_chrset = ""; // character set

I'll explain its importance later.

3. Open() method. Suggested order of parameters:

$dbname $host, $user, $pass, $chrset, $pcon

in most common scenarios our call will be wery simple

Open(); // when all values set as defaults within the
// class
Open($dbname); // in most common scenario, when connecting to
// to different databases within the same server

4. To allow connections with no database selected we'll need a new method
DBSelect() (this name is a suggestion only) to handle database
selection separately or conditionaly within Open() method.

5. DBSelect() method example and its usage within Open() method:

public function DBSelect($dbname="",$chrset="")
{
$retvalue = true;
if (!$dbname) $dbname = $this->db_dbname;
if (!$chrset) $chrset = $this->db_chrset;
$this->error_desc = "";
if (!(mysql_select_db($dbname)))
{
// db select failed
@mysql_close($this->link_id);
$this->error_desc = "Database not found";
$retvalue = false; // error
} else {
if ((strlen($chrset) > 0))
{
if (!(mysql_query("SET CHARACTER SET '{$chrset}'",
$this->link_id)))
{
@mysql_close($this->link_id);
$this->error_desc = "Wrong character set";
$retvalue = false; // error
}
}
}
return $retvalue;
}

changes to Open() method:

// after server connection successfuly established and
// strlen($dbname) > 0
if (!($this->DBSelect($dbname,$chrset)))
{
// db select failed
@mysql_close($this->link_id);
return false; // error
}

6. Variable $chrset. As unicode becomes an every day reality we should be
able to switch database character sets (eg. 'utf8'). See the example of
DBSelect method above.

That's all. I like this class. After implementation of the above changes I
use it in production already.

Frank P. Walentynowicz

GREAT SUGGESTIONS! I completely agree and have already added your ideas into the class.

Open($dbname, $host, $username, $password, $charset, $pcon)
SelectDatabase($dbname, $charset)

Thank you for this contribution,
Jeff

Jeff,

You work amazingly fast! I'm very pleased that you've considered my
suggestions. There is still one thing about the new code:

The following block of code should be removed from Open() method:

// select database
$result = mysql_select_db($dbname);
if (!$result)
{
// db select failed
@mysql_close($this->link_id);
$this->error_desc = "Database not found";
return false; // error
} else {
$this->error_desc = "";
}

Why? It is redundant because of the block which immediately follows. To
choose no selection of db we have to set $dbname to "". Sure enough it
will keep our connection open and it wouldn't select db but it will
set "$this->error_desc" to "Database not found" which is misleading
as it was our intention not to select db. It will also return from Open()
method with "false" which is even more misleading. Open() should return
"false" if the connection attempt fails or db selection fails when
requested. The code which follows takes care of this situation. It
attempts to select db only if $dbname is not "". If $dbname is set to
something and it still fails, the error message will be set to describe
the problem.

I would also suggest adding a line:

$this->error_desc = "Requested database not found";

after the comment "// db select failed" in the block code starting with
"// select a database if specified"

Hope that you don't mind all this fuss. Happy coding!

Frank

I encourage suggestions and comments to make the class the best and easiest possible. I also added your name to the credits under today's date.

Good point. I removed the redundant code and that error message is better. I like the class even better today. That was a very powerful addition.

Good stuff... I ran my test script and it passed with flying colors.

Thanks Frank,
Jeff

Thanks Jeff. I did some method testing of the class and found the problem
with QueryTimed:

$this->Start();
$result = $this->Query($sql);
$this->Stop();

should be:

$this->TimerStart();
$result = $this->Query($sql);
$this->TimerStop();

Frank

Good find. I had changed the method names and didn't catch it.

The class has been updated.

I'm really liking PHP. This is my second PHP class ever - though I have a programming background. I'm transitioning over to open source and it's miles better. Why didn't I find PHP earlier?

This class was a combination of ideas from all the MySQL classes I could find out on the internet or in other libraries. It taught me a lot of how PHP and MySQL calls worked. Almost none of them supported transaction processing and very few would track errors efficiently - which is why I wrote this one. I thought about using a recordset object at first but I liked the ease and power of PHPs arrays and it's "key/value pair" array that acts like a dictionary object. I didn't really even need exception handling because PHPs database calls are so well done.

Now this database class could even be used in partially administering a MySQL server but is still super-easy enough to do simple queries. The only bit I could think to add would be a method that returned a list of databases. You think this would be a good feature to add or would it cause bloat?

Thanks Frank,
Jeff

As an old saying goes - "Every non-tryvial program has a bug in it."
More cleanup is necessary:

1. default values declaration:
------------------------------
after
private $db_charset = ""; // character set (optional)
add
private $db_pcon = false; // use persistent connection?

2. in __construct():
--------------------
$result = $this->Open("", "", "", "", 0);
should be:
$result = $this->Open("", "", "", "", "", 0);

3. in Open():
-------------
add
if (!$charset) $charset = $this->db_charset;
if (!$pcon ) $pcon = $this->db_pcon;
after
// use defaults?
if (!$dbname ) $dbname = $this->db_dbname;
if (!$host ) $host = $this->db_host;
if (!$user ) $user = $this->db_user;
if (!$pass ) $pass = $this->db_pass;

4. in SelectDatabase(): (mea culpa!)
------------------------------------
method declaration should be:
public function SelectDatabase($dbname, $charset="")
in this method $dbname is mandatory and default value = "" doesn't make
any sense (however it makes sense in Open())
remove line:
if (!$dbname) $dbname = $this->db_dbname;

Couple of other things if you don't mind:
-----------------------------------------
Naming convention. In relational database model terminology there is no
place for FILE, RECORD, and FIELD. I know, even mySQL function
names (eg. mysql_field_name, ,mysql_field_len,... etc.) and mySQL manual
are infested with these terms. Proper terms are: TABLE, ROW, and COLUMN.
Writing a new class gives us a chance to hide (of course we cannot change
mySQL function names) improperly used terminology from the developers
and users. May I suggest changing names of some methods to reflect the
true spirit of RDBMS? For example: Records() -> Rows(), RecordsArray() ->
RowsArray() (note that for single entities you've used Row() and RowArray()
already), GetFields() -> GetColumns(), GetFieldLength() ->
GetColumnLength(), GetFieldID() -> GetColumnID(). Maybe I sound like an
old grumpy man (who I am, of course, lol) but words carry meanings and misused cause misunderstanding and chaos.

As of the question if to add a new method to get database names, well, now
that opens another can of worms. IMHO the purest form of our class should
consist of the following: constructor, destructor, OpenConnection(),
SelectDatabase(), Query(), and CloseConnection(). Once the connection to
the server is open what cannot be done with Query() for databaseless
activities? Once the database is selected what cannot be done with Query()?
Anything more are 'pizzas'. Now, what will stop us to create separate,
extended classes based on our class, to provide a specialized functionality
for other purposes. For example (names are symbolic):

base.mysql.class
a) mysql.for.dba.class (inherits from base and provides 'pizzas' for developers like DDL activities and SCHEMA related queries)
b) mysql.for.apps.class (inherits from base and provides an application
famework)

that would be more like OOP. Each class in /includes. How does that sound?

Frank

Again, great stuff. I've added your suggestions to the class and updated the documentation to match. I've actually followed the column vs. field thing for years so it only makes sense to update those method calls.

Good ideas about the base class stuff. I'm thinking I might write a different version that would be a little more complicated to learn (because it would contain multiple classes) but more useful to the strict OOP guys. I've gotten several emails on this already.

Thanks,
Jeff

Hi Jeff,

Put these lines at the beginning of GetHTML():

$tb = "border-collapse:collapse;empty-cells:show";
$th = "border-width:1px;border-style:solid;background-color:navy;color:white";
$td = "border-width:1px;border-style:solid";

then replace line with <table>
with
$html .= "<table style=\"$tb\" cellpadding=\"2\" cellspacing=\"2\">\n";

replace first line with <td> (header)
with
$html .= "\t\t<td style=\"$th\"><strong>" . htmlspecialchars($key) . "</strong></td>\n";

and the second line with <td>
with
$html .= "\t\t<td style=\"$td\">" . htmlspecialchars($value) . "</td>\n";

that will make output nicer.

Frank

I found a bug. In Query($sql="") line number 248, you have a 'return;'. it should be a 'return $this->last_result;' If you do anything but an insert of select statement, such as update, delete, set password..., and you look at the return of the Query function as a quick error check, you will think that you have an error when you don't.


public function Query($sql="")
{
$this->last_sql = $sql;
$this->last_result = @mysql_query($sql, $this->link_id);

if($this->last_result)
{
if(ereg("^insert", strtolower($sql)))
{
$this->last_insert_id = mysql_insert_id();
$numrows = 0;
$this->active_row = -1;
return $this->last_result;
}
elseif(ereg("^select", strtolower($sql)))
{
$numrows = mysql_num_rows($this->last_result);
if ($numrows > 0)
{
$this->active_row = 0;
} else {
$this->active_row = -1;
}
$this->last_insert_id = 0;
return $this->last_result;
}
else
{
return; // This line, sb return $this->last_result;
}
}
else
{
$this->active_row = -1;
$this->error_desc = "Query failed: ($sql)";
return 0; // error
}
}