fix: PostgreSQL getVersion() logic #7488

marekmosna · 2023-05-09T13:05:16Z

Description
Fixes #7484

Fixies logic of PostgreSQL getVersion() method.

ddevsr

Remove below code in phpstan-baseline.neon.dist

-
			message: "#^Strict comparison using \\=\\=\\= between array<string, int|string|null> and false will always evaluate to false\\.$#"
			count: 1
			path: system/Database/Postgre/Connection.php

ddevsr · 2023-05-10T01:08:38Z

By the way,

Following step how to create PR in https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#php-style

Dont forget signing your commit
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

michalsn · 2023-05-11T19:31:08Z

@marekmosna Almost there! Could you follow the suggestion from #7488 (review) to fix the PHPStan error?

kenjis · 2023-05-12T02:35:45Z

system/Database/Postgre/Connection.php

            $this->initialize();
        }

+        $pgVersion = pg_version($this->connID);
+
        return isset($pgVersion['server']) ? $this->dataCache['version'] = $pgVersion['server'] : false;


The interface is public function getVersion(): string;. So false should not be returned.

kenjis · 2023-05-17T00:34:06Z

system/Database/Postgre/Connection.php

            $this->initialize();
        }

-        return isset($pgVersion['server']) ? $this->dataCache['version'] = $pgVersion['server'] : false;
+        $pgVersion                  = pg_version($this->connID);
+        $this->dataCache['version'] = $pgVersion['server'] ?? 'PostgreSQL 7.4 or lower';


getVersion() returns the version number like '12.1', and the number should be used with version_compare()

Is it impossible to get the version number 7.4 or lower?

pg_version() returns an array with the client, protocol and server version. Protocol and server versions are only available if PHP was compiled with PostgreSQL 7.4 or later.
https://www.php.net/manual/en/function.pg-version.php#refsect1-function.pg-version-description

There is no version requirement in the docs:
https://codeigniter4.github.io/CodeIgniter4/intro/requirements.html#supported-databases

But PostgreSQL 11 will stop receiving fixes on November 9, 2023.
https://www.postgresql.org/about/news/postgresql-153-148-1311-1215-and-1120-released-2637/

@codeigniter4/core-team @codeigniter4/database-team
Do we need to support versions prior to 7.4?
Why don't we add (version 7.4 and above only) in the docs?

PostgreSQL 7.4 seems too old, it seems it need to be documented in docs

getVersion() returns the version number like '12.1', and the number should be used with version_compare()

that was the question in the related issue #7488

If there is no $pgVersion['server'], we should return an empty string.

Adding a note (version 7.4 and above only) is a good idea.

I sent a PR: #7507

kenjis · 2023-05-18T07:57:15Z

system/Database/Postgre/Connection.php

            $this->initialize();
        }

-        return isset($pgVersion['server']) ? $this->dataCache['version'] = $pgVersion['server'] : false;
+        $pgVersion                  = pg_version($this->connID);
+        $this->dataCache['version'] = $pgVersion['server'] ?? 'PostgreSQL 7.4 or lower';


Suggested change

$this->dataCache['version'] = $pgVersion['server'] ?? 'PostgreSQL 7.4 or lower';

$this->dataCache['version'] = $pgVersion['server'] ?? '';

kenjis · 2023-05-18T08:01:26Z

@marekmosna It seems there is no test code for getVersion().
If you can write test code, add test code.
We would appreciate it.

marekmosna · 2023-05-20T07:16:47Z

@marekmosna It seems there is no test code for getVersion(). If you can write test code, add test code. We would appreciate it.

I'm sorry @kenjis but I'm not a php developer so I can't

kenjis · 2023-05-20T10:30:00Z

@marekmosna Okay, I got it.

michalsn

LGTM!

kenjis

LGTM!

kenjis · 2023-05-21T03:21:07Z

@codeigniter4/database-team
It seems PostgreSQL may return the version like '15.3 (Debian 15.3-1.pgdg110+1)'.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/5035338925/jobs/9030854318

Is this okay?

michalsn · 2023-05-21T07:15:21Z

Good point. I think we should filter out the version.

michalsn · 2023-05-21T07:17:30Z

system/Database/Postgre/Connection.php

            $this->initialize();
        }

-        return isset($pgVersion['server']) ? $this->dataCache['version'] = $pgVersion['server'] : false;
+        $pgVersion                  = pg_version($this->connID);
+        $this->dataCache['version'] = $pgVersion['server'] ?? '';


Suggested change

$this->dataCache['version'] = $pgVersion['server'] ?? '';

$this->dataCache['version'] = isset($pgVersion['server']) ?

preg_match('/^(\d+\.\d+)/', $pgVersion['server'], $matches) ? $matches[1] : '' :

'';

This should work if we deal with something like 15.3 (Debian 15.3-1.pgdg110+1) - as Kenjis pointed out earlier.

Okay, I will take care of it in #7509

ddevsr suggested changes May 10, 2023

View reviewed changes

ddevsr added the bug Verified issues on the current code behavior or pull requests that will fix them label May 10, 2023

marekmosna force-pushed the 7484-postgresql-server-version branch 2 times, most recently from 7ee8775 to 8064aab Compare May 11, 2023 16:44

kenjis added the database Issues or pull requests that affect the database layer label May 12, 2023

kenjis changed the title ~~Fixes 7484: PostgreSQL getVersion() logic~~ fix: PostgreSQL getVersion() logic May 12, 2023

kenjis reviewed May 12, 2023

View reviewed changes

marekmosna force-pushed the 7484-postgresql-server-version branch from 98362e6 to efbf68c Compare May 12, 2023 18:06

kenjis added the tests needed Pull requests that need tests label May 12, 2023

marekmosna force-pushed the 7484-postgresql-server-version branch from efbf68c to 3292e43 Compare May 13, 2023 11:05

kenjis reviewed May 17, 2023

View reviewed changes

kenjis mentioned this pull request May 17, 2023

docs: require PostgreSQL 7.4 or later #7507

Merged

2 tasks

kenjis reviewed May 18, 2023

View reviewed changes

Fix PostgreSQL getVersion() logic

4c6e4ec

marekmosna force-pushed the 7484-postgresql-server-version branch from 3292e43 to 4c6e4ec Compare May 20, 2023 07:13

michalsn approved these changes May 20, 2023

View reviewed changes

kenjis approved these changes May 21, 2023

View reviewed changes

kenjis merged commit d334a2a into codeigniter4:develop May 21, 2023

kenjis mentioned this pull request May 21, 2023

fix: PostgreSQL getVersion() output #7509

Merged

4 tasks

michalsn reviewed May 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: PostgreSQL getVersion() logic #7488

fix: PostgreSQL getVersion() logic #7488

marekmosna commented May 9, 2023 •

edited by kenjis

Loading

ddevsr left a comment

ddevsr commented May 10, 2023

michalsn commented May 11, 2023

kenjis May 12, 2023

kenjis May 17, 2023

kenjis May 17, 2023

samsonasik May 17, 2023

marekmosna May 17, 2023

michalsn May 17, 2023

kenjis May 17, 2023

kenjis May 18, 2023

kenjis commented May 18, 2023

marekmosna commented May 20, 2023

kenjis commented May 20, 2023

michalsn left a comment

kenjis left a comment

kenjis commented May 21, 2023

michalsn commented May 21, 2023

michalsn May 21, 2023

kenjis May 21, 2023

	$this->dataCache['version'] = $pgVersion['server'] ?? 'PostgreSQL 7.4 or lower';
	$this->dataCache['version'] = $pgVersion['server'] ?? '';

-        $this->dataCache['version'] = $pgVersion['server'] ?? '';
+        $this->dataCache['version'] = isset($pgVersion['server']) ?
+                preg_match('/^(\d+\.\d+)/', $pgVersion['server'], $matches) ? $matches[1] : '' :
+                '';

fix: PostgreSQL getVersion() logic #7488

fix: PostgreSQL getVersion() logic #7488

Conversation

marekmosna commented May 9, 2023 • edited by kenjis Loading

ddevsr left a comment

Choose a reason for hiding this comment

ddevsr commented May 10, 2023

michalsn commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis commented May 18, 2023

marekmosna commented May 20, 2023

kenjis commented May 20, 2023

michalsn left a comment

Choose a reason for hiding this comment

kenjis left a comment

Choose a reason for hiding this comment

kenjis commented May 21, 2023

michalsn commented May 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekmosna commented May 9, 2023 •

edited by kenjis

Loading