Minor fixes to avoid PHP warnings in v8.2+

[Mike-Benoit]

Minor fixes to avoid PHP warnings in v8.2+.

See comments in code for further details if its not a simple object property initialization.

[dregad]

Wouldn't it be more appropriate to add this fix in ADORecordSet::Close() ?

[Mike-Benoit]

Agreed, that would make more sense.

[dregad]

I'm still not sure that is the right fix actually, as this will prevent execution of cleanup tasks (e.g. freeing additional result sets in the mysql driver).

I believe that each driver should handle this specifically to ensure they don't break when _queryID == -1. At least some of them already do that (but I didn't check them all):

• mysqli

  ADOdb/drivers/adodb-mysqli.inc.php

  Lines 1644 to 1646 in 1a008f9

  	if ($this->_queryID instanceof mysqli_result) {
  	mysqli_free_result($this->_queryID);
  	}

• postgres

  ADOdb/drivers/adodb-postgres64.inc.php

  Lines 1095 to 1097 in 1a008f9

  	if ($this->_queryID === false || $this->_queryID == self::DUMMY_QUERY_ID) {
  	return true;
  	}

• oci8

  ADOdb/drivers/adodb-oci8.inc.php

  Lines 1837 to 1838 in 1a008f9

  	if (is_resource($this->_queryID))
  	@oci_free_statement($this->_queryID);

DB2 and firebird don't have such checks.

Was this just preventive maintenance, or did you run into an actual error or warning ?

[Mike-Benoit]

We definitely hit an error here, passing a string where a connection object is required.

Our internal ADODB branch has been out-of-sync with upstream for over a decade now. It's possible we missed some upstream changes that resolved some of our issues in other ways.

These are some of the last bits to bring it back in sync though, so that will be nice. We can leave this out for now and we will run further tests and let you know if there still a problem.

Thanks for your prompt response with these PR's.

[dregad]

You're welcome. So should I close this PR then ?

[Mike-Benoit]

Sure

[dregad]

OK with the csvlib and postgres driver changes, I'll go ahead commit those separately.

As for bogus queryID, more discussion is needed

[dregad]

I'm still not sure that is the right fix actually, as this will prevent execution of cleanup tasks (e.g. freeing additional result sets in the mysql driver).

I believe that each driver should handle this specifically to ensure they don't break when _queryID == -1. At least some of them already do that (but I didn't check them all):

• mysqli

  ADOdb/drivers/adodb-mysqli.inc.php

  Lines 1644 to 1646 in 1a008f9

  	if ($this->_queryID instanceof mysqli_result) {
  	mysqli_free_result($this->_queryID);
  	}

• postgres

  ADOdb/drivers/adodb-postgres64.inc.php

  Lines 1095 to 1097 in 1a008f9

  	if ($this->_queryID === false || $this->_queryID == self::DUMMY_QUERY_ID) {
  	return true;
  	}

• oci8

  ADOdb/drivers/adodb-oci8.inc.php

  Lines 1837 to 1838 in 1a008f9

  	if (is_resource($this->_queryID))
  	@oci_free_statement($this->_queryID);

DB2 and firebird don't have such checks.

Was this just preventive maintenance, or did you run into an actual error or warning ?

[dregad]

You might want to update your branch to fix the conflict I introduced by modifying the comment in the postgres64 driver.

[dregad]

Sorry Mike, I just realized that when I cherry-picked the changes (059ffc2 and b881dea) I forgot to update the commits' author to credit you for them. My apologies.

[Mike-Benoit]

No worries at all.

[dregad]

Closing this, as 3 of the changes have been covered separately (see #954, #956, #957).
The remaining point will be remain as-is for now (see discussion #952 (comment))