core: method adorecordset::fields incorrect documentation, inconsistent across drivers, unstable

[mnewnham]

The method adorecordset::fields() currently has the following issues:

Issue

Documentation

The documentation indicates that the method takes a single integer value (the offset of a field) as an argument, In fact the method takes a single string value, the column name.

Inconsistency

The method never works correctly when the driver fetch mode is set to ADODB_FETCH_NUM, but some drivers attempt to find a field any way, resulting in PHP warnings.

Stability

The method makes no attempt to validate the passed column name, resulting in a PHP warning if an invalid column name is passed.

Incorrect Usage

The unsupported ado driver uses fields in the method shown in the documentation. This is the likely cause of the error

Resolution

1. Documentation to be updated
2. The return value from this method to be standardized to return false if
   a. The fetch mode is currently ADODB_FETCH_NUM
   b. An invalid column name is provided.
3. Code should test the return value === false to determine if an error has occurred.

Fixes will be packaged with the code to resolve #747

[dregad]

The method never works correctly when the driver fetch mode is set to ADODB_FETCH_NUM

In fact the PHPDoc states Will not work if ADODB_FETCH_MODE is set to ADODB_FETCH_NUM, but this was not reflected in the documentation.

The return value from this method to be standardized to return false

Maybe throw an exception (adodb_throw) ?

Fixes will be packaged with the code to resolve #747

Are you working on #747 ? Just making sure we don't duplicate efforts, as I have some work-in-progress on that.

[mnewnham]

I started work on #747 yesterday evening, Did I miss you assigned? in which case I apologize. The work I had started was to standardize the mssqlnative and oracle code as previously assigned. I became distracted by this problem with fields()

[mnewnham]

I've pushed my work so far in the branch "field-types-array-refactor-747" if you want to check to see if there are any conflicts with what you are doing. There may be debugging messages as the work is incomplete.

[dregad]

Did I miss you assigned?

No you did not; I should indeed have assigned myself when I opened this to avoid such situation.

For the record, this works predates #747, I actually started working on it before I got sidetracked between May and August 2021, but to be honest I am not actively on it.

Note that I do not want to cause delays, and I'm pretty sure your fix will be as good what I was doing so I can probably drop that, or make some follow-up commits if necessary.

branch "field-types-array-refactor-747" if you want to check to see if there are any conflicts

I'll have a look, thanks but do feel free to carry on.

[dregad]

I did not actually test your code, but it looks good to me, we basically followed the same approach.

The key difference is that I declared a protected _fetchField() stub method (just returning false) in the parent ADORecordset class, which ensures consistency in child classes that override it - currently it's sometimes private (firebird), protected (mssql, postgresql) or even public (oracle).

Also, I modified ADORecordset::fetchField() check if the cache is empty and populate it if so, this way we can get rid of this method in at least some of the child classes.

I don't think the protected $fieldObjectsRetrieved = false; property is necessary, it should be sufficient to check whether $fieldObjectsCache is empty.

For reference, my code is here https://github.com/dregad/ADOdb/tree/i747-cache-field-metadata (just rebased on top of latest master)

Since your work seems to be in a more advanced state (mine only covers the PostgreSQL driver and at the moment), it makes sense to continue with yours.

[dregad]

@mnewnham where do you stand with this ? Let me know if you would like me to delay 5.22.0 until you're done.

[mnewnham]

I haven't finished this. If you release, I'll rebase it onto master for 5.23