Convert resources to objects in ext/pgsql by kocsismate · Pull Request #6791 · php/php-src

kocsismate · 2021-03-21T21:13:26Z

No description provided.

ext/pgsql/pgsql.stub.php

kocsismate · 2021-03-21T21:54:47Z

ext/pgsql/pgsql.c

 #define pg_encoding_to_char(x) "SQL_ASCII"
 #endif

-/* {{{ _php_pgsql_trim_message */


I removed some of these unnecessary comments because they annoyed me at some point

This does make a lot of noise in the PR sadly :-/

I agree that it was not a good idea to do this here. I can get rid of these changes If they make code review difficult

Feel free to simply separately land a removal of all folder marks in pgsql (though I don't mind it being part of this change).

I'll do it separately!

Girgias

Looks OK from a quick glance if persistent connections still work (as currently there is no plan nor deprecation getting rid of those)

ext/pgsql/pgsql.c

nikic · 2021-03-22T09:15:51Z

/home/vsts/work/1/s/ext/pgsql/pgsql.c:1515:10: error: unused variable ‘pg_link’ [-Werror=unused-variable]

kocsismate · 2021-03-24T23:53:17Z

Now only ~12 tests fail and I also tried to implement the suggestionss in in c7a86a3 (notices doesn't yet work perfectly).

ext/pgsql/pgsql.c

kocsismate · 2021-04-02T18:24:11Z

My latest commit (6931655) fixes all tests, finally!

nikic · 2021-04-26T12:29:55Z

Test failure is legit: https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=16129&view=ms.vss-test-web.build-test-results-tab&runId=369662&resultId=109155&paneView=debug

NEWS

ext/pgsql/pgsql.stub.php

ext/pgsql/tests/80_bug32223b.phpt

ext/pgsql/pgsql.c

ext/pgsql/php_pgsql.h

nikic · 2021-04-26T14:28:22Z

ext/pgsql/pgsql.c

 #define pg_encoding_to_char(x) "SQL_ASCII"
 #endif

-/* {{{ _php_pgsql_trim_message */


Feel free to simply separately land a removal of all folder marks in pgsql (though I don't mind it being part of this change).

ext/pgsql/pgsql.c

kocsismate · 2021-05-10T08:54:54Z

I addressed most of the review comments, however, currently many tests end up with a memory leak. The issue is with the default connection handling, so I'll continue chasing this problem.

nikic · 2021-05-10T09:59:21Z

I pushed a few changes that should fix the leaks.

nikic · 2021-05-10T12:55:25Z

Windows build failed with:

========DIFF========
002+  
     int(%d)
     int(%d)
========DONE========
FAIL Bug #72197 pg_lo_create arbitrary read [C:\projects\php-src\ext\pgsql\tests\bug72197.phpt]

Not really obvious to me why ...

nikic · 2021-05-10T12:56:25Z

ext/pgsql/pgsql.c

-	}
-
-	if ((pgsql = (PGconn *)zend_fetch_resource2(link, "PostgreSQL link", le_link, le_plink)) == NULL) {
+		zend_argument_type_error(1, "must be of type PgSql when the connection is provided");


Suggested change

zend_argument_type_error(1, "must be of type PgSql when the connection is provided");

zend_argument_type_error(1, "must be of type PgSql\\Connection when the connection is provided");

This one isn't resolved yet.

ext/pgsql/tests/bug72197.phpt

ext/pgsql/pgsql.c

ext/pgsql/php_pgsql.h

ext/pgsql/pgsql.c

…w tests

The idea came up in php@c7a86a3

kocsismate · 2021-05-10T20:54:15Z

I had to force push because of a conflict in the readme, but only the last commit is new.

nikic · 2021-05-10T21:19:44Z

ext/pgsql/pgsql.c

-	}
-
-	if ((pgsql = (PGconn *)zend_fetch_resource2(link, "PostgreSQL link", le_link, le_plink)) == NULL) {
+		zend_argument_type_error(1, "must be of type PgSql when the connection is provided");


This one isn't resolved yet.

kocsismate · 2021-05-10T22:13:43Z

This one isn't resolved yet.

Did it when applying the commit!

kocsismate force-pushed the pgsql-resource branch from 777ebcf to 1c1b974 Compare March 21, 2021 21:23

kocsismate commented Mar 21, 2021

View reviewed changes

kocsismate force-pushed the pgsql-resource branch from 1c1b974 to 5b34373 Compare March 21, 2021 21:55

Girgias reviewed Mar 22, 2021

View reviewed changes

ext/pgsql/pgsql.c Show resolved Hide resolved

ext/pgsql/pgsql.c Show resolved Hide resolved

ext/pgsql/pgsql.c Outdated Show resolved Hide resolved

ext/pgsql/pgsql.c Outdated Show resolved Hide resolved

kocsismate marked this pull request as draft March 22, 2021 09:17

kocsismate force-pushed the pgsql-resource branch 2 times, most recently from 9c9bb86 to 45f55f7 Compare March 24, 2021 23:51

kocsismate commented Mar 24, 2021

View reviewed changes

ext/pgsql/pgsql.c Show resolved Hide resolved

kocsismate mentioned this pull request Mar 25, 2021

Resource to opaque object migration php/php-tasks#6

Open

41 tasks

kocsismate marked this pull request as ready for review March 30, 2021 07:57

kocsismate force-pushed the pgsql-resource branch from 45f55f7 to 6a929c9 Compare April 2, 2021 18:17

kocsismate force-pushed the pgsql-resource branch 2 times, most recently from 6931655 to eb22b17 Compare April 2, 2021 19:29

kocsismate force-pushed the pgsql-resource branch 2 times, most recently from dbf23ba to 98c2c0d Compare April 20, 2021 22:38

nikic reviewed Apr 26, 2021

View reviewed changes

ramsey added the Feature label May 8, 2021

kocsismate added this to the PHP 8.1 milestone May 8, 2021

kocsismate force-pushed the pgsql-resource branch from 98c2c0d to c05c20b Compare May 10, 2021 08:46

nikic reviewed May 10, 2021

View reviewed changes

ext/pgsql/tests/bug72197.phpt Show resolved Hide resolved

nikic reviewed May 10, 2021

View reviewed changes

ext/pgsql/pgsql.c Outdated Show resolved Hide resolved

nikic reviewed May 10, 2021

View reviewed changes

ext/pgsql/pgsql.c Outdated Show resolved Hide resolved

ext/pgsql/pgsql.c Outdated Show resolved Hide resolved

ext/pgsql/php_pgsql.h Outdated Show resolved Hide resolved

ext/pgsql/pgsql.c Outdated Show resolved Hide resolved

ext/pgsql/pgsql.c Outdated Show resolved Hide resolved

kocsismate and others added 14 commits May 10, 2021 22:48

Convert resources to objects in ext/pgsql

47a5635

Fix build and remove outdated comment

f313d5a

Get rid of memory leak, improve default connection handling, fix a fe…

0d52aa7

…w tests

Get rid of the hashes global hash

50298ad

The idea came up in php@c7a86a3

Get rid of global notices hash

b86b71d

The idea came up in php@c7a86a3

New test fixes

efd1049

Address review comments

a745eb5

Some fixes

94f5756

Some memory management fixes

3781407

Fix connection reuse

be1161b

Minor cleanup

6553e24

Remove useless funcinfo

51e861f

Fix test and error messages

7fd8e01

Address review comments

d735e83

kocsismate force-pushed the pgsql-resource branch from 4acde1a to d735e83 Compare May 10, 2021 20:49

nikic approved these changes May 10, 2021

View reviewed changes

kocsismate closed this in 32aff25 May 10, 2021

kocsismate deleted the pgsql-resource branch May 10, 2021 22:11

This was referenced Sep 21, 2021

PHP 8.1 support laminas/laminas-session#45

Merged

Recognize pgsql resource objects when running in PHP 8.1 laminas/laminas-db#212

Merged

kocsismate mentioned this pull request Sep 25, 2023

Migrate ext/odbc resources to opaque objects #12040

Merged

jrfnl mentioned this pull request Aug 14, 2024

PHP 8.1 | NewClasses: handle classes introduced for resource to object conversion PHPCompatibility/PHPCompatibility#1733

Merged

	zend_argument_type_error(1, "must be of type PgSql when the connection is provided");
	zend_argument_type_error(1, "must be of type PgSql\\Connection when the connection is provided");

Conversation

kocsismate commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic commented Mar 22, 2021

Uh oh!

kocsismate commented Mar 24, 2021

Uh oh!

Uh oh!

kocsismate commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Apr 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented May 10, 2021

Uh oh!

nikic commented May 10, 2021

Uh oh!

nikic commented May 10, 2021

Uh oh!

nikic May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented May 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kocsismate commented Mar 21, 2021 •

edited

Loading

kocsismate commented Apr 2, 2021 •

edited

Loading

nikic May 10, 2021 •

edited

Loading