Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b1f583
Minor fixes to avoid PHP warnings in v8.2+
Apr 12, 2023
82a9860
Suggested improvements.
Apr 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion adodb.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4023,8 +4023,11 @@ class ADORecordSet implements IteratorAggregate {
var $_atLastPage = false; /** Added by Iván Oliva to implement recordset pagination */
var $_lastPageNo = -1;
var $_maxRecordCount = 0;
var $rowsPerPage;
Mike-Benoit marked this conversation as resolved.
Show resolved Hide resolved
var $datetime = false;

var $oldProvider;

Mike-Benoit marked this conversation as resolved.
Show resolved Hide resolved
public $customActualTypes;
public $customMetaTypes;

Expand Down Expand Up @@ -4064,7 +4067,10 @@ function __construct($queryID,$mode=false) {
}

function __destruct() {
$this->Close();
//If the _queryID is bogus, calling Close() may cause an error in the database driver.
if($this->_queryID != -1) {
$this->Close();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that would make more sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

DB2 and firebird don't have such checks.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

}

#[\ReturnTypeWillChange]
Expand Down
4 changes: 3 additions & 1 deletion drivers/adodb-postgres64.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ function qStr($s, $magic_quotes=false)
if ($this->_connectionID) {
return "'" . pg_escape_string($this->_connectionID, $s) . "'";
} else {
return "'" . pg_escape_string($s) . "'";
//If there is no database connection, then pg_escape_string() will not work in PHP 8.3+.
// This can occur when using setSessionVariables() in the load balancer, so fall back to emulated escaping in these cases.
return parent::qStr( $s );
Mike-Benoit marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down