Merge pull request #1 from phppgadmin/master#118
Merge pull request #1 from phppgadmin/master#118idontusenumbers wants to merge 7 commits intophppgadmin:masterfrom
Conversation
| /** | ||
| * Set server information. | ||
| * @param $key parameter name to set, or null to replace all | ||
| * @param $key string parameter name to set, or null to replace all |
There was a problem hiding this comment.
IDEA/PHP Storm was complaining about the paramater types not matching "expecting the but was string" etc.
| } | ||
| $this->setServerInfo('platform', $platform, $server_id); | ||
| $this->setServerInfo('pgVersion', $_connection->conn->pgVersion, $server_id); | ||
| $this->setServerInfo('pgVersion',/* $_connection->conn->pgVersion*/ "unknown version", $server_id); |
There was a problem hiding this comment.
This variable wasn't defined and I'm not sure where it ever lived. I simply removed it and phppgadmin seems to still properly report the version.
There was a problem hiding this comment.
I believe #125 is a better fix for that one.
| */ | ||
| function __construct($host, $port, $sslmode, $user, $password, $database, $fetchMode = ADODB_FETCH_ASSOC) { | ||
| $this->conn = ADONewConnection('postgres7'); | ||
| $this->conn = ADONewConnection('postgres'); |
There was a problem hiding this comment.
ADODb says to reference just postgres to grab the latest driver
|
Most of these changes are just upgrading ADOdb; I wasn't able to identify the cause using the existing version; I saw the ADODb library was out of date and thought upgrading that would do the trick since the error was in that code. |
| */ | ||
| function __construct() | ||
| { | ||
| die('Virtual Class -- cannot instantiate'); |
There was a problem hiding this comment.
This seems to be the origin of the error
|
Any update/news on this PR? Have the same issue with the following setup:
|
|
Hmm, as far as I understand this error occurs due to incorrect inheritance. The |
For version 7.13.0 try using this patch. |
Is there any reason to have the override if it's not doing anything? |
Yes, of course there is. This constructor is required so that the constructor of the inherited |
works like a charm. thanks |
|
Why it still not merged? |
6 participants
Fixes #117
Running the tests looked like a lot of work so I apologize but I didn't run them (I might suggest a docker-compose based solution for running the tests that can be run in one command). Feel free to include these changes in your own commit and reject the PR.
I tested it connects and appears to function with my
postgres:12-alpinecontainer.