[core] Improve error messaging in getUpdateSql() / getInsertSql()

[mnewnham]

Current functionality

If an invalid statement is passed to getInsertSql() then a message

>Bad $rs in getInsertSql. Connection or SQL invalid. Try using $connection->debug=true;

The lack of information in the message makes debugging difficult, particularly if multiple connections are being used simultaneously. Improving the functionality would help.

Improved functionality

Default Message

Change the message to include a driver and table name

[driver.table] Bad $rs in getInsertSql. Connection or SQL invalid. Try using $connection->debug=true;

Debug Mode

Add message to ADOConnection::outp()

[NathanGibbs3]

• Change the default message defined in the ADODB_BAD_RS constant, adding %s placeholders for driver & table.

Change the calls using the constant in:

• adodb-lib.inc.php
  • Driver
  • Table - Same question as below.
• tohtml.inc.php

In the file tohtml.inc.php, how would we handle this in the rs2html() function?
The dataProvider and tableName would be passed on $rs, which is bad, so those properties are not available.

1. All I can think of right now is printf(ADODB_BAD_RS, 'Unknown', 'Unknown', rs2html'); That is about as unhelpful as the original error message.
2. Drop the printf completely from that function. Which is less helpful than the original error message.

Any Ideas?

[NathanGibbs3]

Unless I am seriously missing something in the code, I don't think we can get a table name if the $rs is bad.
The code gets the table from $rs and we only use this message if $rs is false.

[NathanGibbs3]

Debug Mode

Add message to ADOConnection::outp()

What exactly do we need here?

I took it to mean:

1. Change the printf calls to ADOConnection::outp(sprintf()) calls.
2. Wrap them in if ($zthis->debug) { blocks.

This of course defeats the whole ending of the error message, Try using $connection->debug=true;, as the user would never see that until setting $debug.

I think I have the wrong idea, please clarify?

[dregad]

I'm not really sure what @mnewnham had in mind when he opened this, but I agree with your assessment with regards to rs2html().

For getUpdateSql() / getInsertSql() at least we have the ADOConnection object available ($zthis parameter) so some extra information could be provided based on that.

Hopefully he'll care to comment / explain.

[NathanGibbs3]

For getUpdateSql() / getInsertSql() at least we have the ADOConnection object available ($zthis parameter) so some extra information could be provided based on that.

Correct, currently in my code I'm using $zthis->dataProvider, $zthis->database to fill in the driver and "table"
For the driver I'm not sure if $zthis->databaseType would be a more appropriate parameter to use.

Hopefully he'll care to comment / explain.

Yes please, once the driver parameter question and the rs2html() question are answered, I'll PR this for you.
I'll also provide backports if you want them.

[NathanGibbs3]

Side Note.
I noticed that rs2html() along with some other functions in the to*.inc.php files are part of the Unsupported Addons.

The tohtml.inc.php & toexport.inc.php files are literally less than 400 lines of code.
I'm willing to provide support for them if you would like.

[NathanGibbs3]

Another thought I had around this as well.
The Error message states Connection or SQL invalid.. It's ambiguous what the actual issue is. With some additional code we could separate the connection issue from the invalid SQL issue and return a more meaningful error message.

Using the rs2html function for example:

    if (!$rs) {
        printf(ADODB_BAD_RS, 'Unknown', 'Unknown', 'rs2html');
        return false;
    }elseif (!$rs->connection) {
        printf(ADODB_BAD_CX, $rs->dataProvider, $rs->tableName, 'rs2html');
        return false;
    }

Of Course ADODB_BAD_RS would have to be reworded to remove reference to invalid connections; and a separate ADODB_BAD_CX error message for bad connections would have to be defined.

This idea is out of scope for Issue #899 . However, if this is something that would be useful in a future release, I could open an issue for it, and implement a solution for it in code fairly quickly.

Thoughts?

[dregad]

I noticed that rs2html() along with some other functions in the to*.inc.php files are part of the Unsupported Addons.

The tohtml.inc.php & toexport.inc.php files are literally less than 400 lines of code. I'm willing to provide support for them if you would like.

Yes of course ! Contributions are always welcome, feel free to send fixes and improvements via pull requests. Thank you very much !

The Error message states Connection or SQL invalid.. It's ambiguous what the actual issue is. With some additional code we could separate the connection issue from the invalid SQL issue and return a more meaningful error message.

It's not like anybody has ever complained about it over the last 15 years... But anyway I'm fine with that idea.

Of Course ADODB_BAD_RS would have to be reworded to remove reference to invalid connections; and a separate ADODB_BAD_CX error message for bad connections would have to be defined.

I'd rather avoid the CX abbreviation, and use ADODB_BAD_CONNECTION instead

This idea is out of scope for Issue #899 . However, if this is something that would be useful in a future release, I could open an issue for it, and implement a solution for it in code fairly quickly.

As mentioned before, contributions are welcome. Looking forward to the new issue and pull request 😃

[NathanGibbs3]

I'll do that at some point and keep it separate from this issue.

Thank you for the feedback. 😄

[NathanGibbs3]

I took it to mean:

1. Change the `printf` calls to `ADOConnection::outp(sprintf())` calls.

2. Wrap them in `if ($zthis->debug) {` blocks.

This of course defeats the whole ending of the error message, Try using $connection->debug=true;, as the user would never see that until setting $debug.

If @mnewnham does check in on this, I can always change the reference to $debug in ADODB_BAD_RS in a future PR. At the moment, I left it as is to follow what I beleive to be the intent of this issue as closely as possible.

[NathanGibbs3]

This of course defeats the whole ending of the error message, Try using $connection->debug=true;, as the user would never see that until setting $debug.

Confirmed while working on a fix for #994