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

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

No description provided.

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

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)

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

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

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

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

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

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.

…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

This one isn't resolved yet.

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

This one isn't resolved yet.

Did it when applying the commit!

Per: - php/php-src#6791 - https://github.com/php/php-src/blob/a846547ed4bb67f00dc12bbfc529e9c992cbfd07/UPGRADING The pgsql functions now accept and return **resource objects** and not **resources**. As such, the return value of `get_resource_type()` varies, and technically, in PHP 8.1, we should not use `is_resource()`, but a typecheck instead. However, we still need to support pre-8.1 code as well. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

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

kocsismate commented Mar 21, 2021
View reviewed changes

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

Girgias reviewed Mar 22, 2021
View reviewed changes

Comment threadext/pgsql/pgsql.c Outdated
Comment threadext/pgsql/pgsql.c Outdated
Comment threadext/pgsql/pgsql.c Outdated
Comment threadext/pgsql/pgsql.c Outdated

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 45f55f7Compare March 24, 2021 23:51

kocsismate commented Mar 24, 2021
View reviewed changes

Comment threadext/pgsql/pgsql.c Outdated

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 6a929c9Compare April 2, 2021 18:17

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

kocsismate force-pushed the pgsql-resource branch 2 times, most recently from dbf23ba to 98c2c0dCompare 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 c05c20bCompare May 10, 2021 08:46

nikic reviewed May 10, 2021
View reviewed changes

Comment threadext/pgsql/tests/bug72197.phpt Outdated

nikic reviewed May 10, 2021
View reviewed changes

Comment threadext/pgsql/pgsql.c Outdated

nikic reviewed May 10, 2021
View reviewed changes

Comment threadext/pgsql/pgsql.c Outdated
Comment threadext/pgsql/pgsql.c Outdated
Comment threadext/pgsql/php_pgsql.h Outdated
Comment threadext/pgsql/pgsql.c Outdated
Comment threadext/pgsql/pgsql.c Outdated

kocsismateand 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 d735e83Compare May 10, 2021 20:49

nikic approved these changes May 10, 2021
View reviewed changes

Comment threadext/pgsql/pgsql.c Outdated
nikicMay 10, 2021
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one isn't resolved yet.

kocsismate closed this in 32aff25May 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");

Uh oh!

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!

nikicMay 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

nikicMay 10, 2021•
edited
Loading