Implement "Deprecate functions with overloaded signatures" RFC for PHP 8.3 by kocsismate · Pull Request #11703 · php/php-src

kocsismate · 2023-07-13T21:39:09Z

Implementation for https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures. Each subvote has a separate commit.

iluuu1994 · 2023-07-13T22:19:45Z

I did not review this yet, but the PR is missing checks for exceptions promoted by error handlers after zend_error(E_DEPRECATED, ...).

Girgias

I made a first pass with some comments, I skipped FFI and Date, but the other ones mostly seem to make sense.

Girgias · 2023-07-14T02:33:31Z

ext/dba/dba.c

+		zend_error(E_DEPRECATED, "Calling dba_fetch() with $dba at the 3rd parameter is deprecated",
+			ZSTR_VAL(fbc->common.function_name));


Passing that char* seems odd?

Ah yes, there used to be a %s instead of dba_fetch(), and I forgot to remove the parameter.

ext/intl/calendar/calendar.stub.php

ext/intl/calendar/gregoriancalendar_methods.cpp

ext/intl/tests/gregoriancalendar_set_date_error.phpt

ext/intl/tests/gregoriancalendar_set_date_time_error.phpt

ext/ldap/ldap.c

kocsismate · 2023-07-16T09:40:58Z

I squashed the fixups because there were too many of them. Sorry for that!

Girgias

I am fine with already merging the DBA, Intl, Reflection, Phar, PGSQL, and stream PRs.

FFI and get_class() has legit CI failures.

ext/intl/tests/calendar_isSet_empty_time.phpt

ext/pgsql/pgsql.c

Girgias · 2023-07-16T17:58:09Z

Actually the intl and postgress also have CI failures on Windows.

kocsismate · 2023-07-16T21:10:38Z

Actually the intl and postgress also have CI failures on Windows.

Thanks for the reminder, I've just tried to fixed them. Turned out that I had a bug in the pgsql code...

I'll also try to fix the FFI memleaks tomorrow. Unfortunately, I cannot reproduce them locally, but at least I have a suspecicion where the issue lies.

Girgias · 2023-07-16T23:01:03Z

I can reproduce the FFI failures with ASAN, and the memory leak is a pre-existing issue, which I assume is related to something not being cleaned-up on failure when the FFI object is being destroyed.

To not have this be blocking, either XFAIL this or revert the 3 tests to use the static method and emit the deprecation notice.

@dstogov (or @bwoebi) I'm imagining the issues is related to the FFI destructors not properly cleaning up after themselves if not called statically. The fix should also be backported to supported versions.

kocsismate · 2023-07-17T08:56:25Z

I can reproduce the FFI failures with ASAN, and the memory leak is a pre-existing issue, which I assume is related to something not being cleaned-up on failure when the FFI object is being destroyed.

Relevant test PR: #11720

iluuu1994

To clarify: It is common for PHP code to register error handlers that promote errors to exceptions. In these cases, the called functions shouldn't actually take effect, i.e. execute, so we need to abort accordingly with `if (UNEXPECTED(EG(exception)) { possibly_cleanup(); RETURN_THROWS(); }

iluuu1994 · 2023-07-17T09:10:31Z

Zend/zend_vm_def.h

+			if (UNEXPECTED(EG(exception))) {
+				HANDLE_EXCEPTION();
+			} else {
+				ZEND_VM_NEXT_OPCODE();


Nit: No need to move this line.

ext/dba/dba.c

ext/ffi/ffi.c

iluuu1994 · 2023-07-17T11:30:36Z

ext/ldap/ldap.c

+	char *wallet = NULL, *walletpasswd = NULL;
+	size_t walletlen = 0, walletpasswdlen = 0;
+	zend_long authmode = GSLC_SSL_NO_AUTH;
+	int ssl = 0;


Could be a bool.

ext/ldap/ldap.c

ext/phar/phar_object.c

ext/reflection/php_reflection.c

…lendar::createFromDateTime()

…eld_is_null() nullable

andypost · 2023-07-19T21:52:26Z

ext/reflection/php_reflection.stub.php

@@ -205,6 +205,8 @@ class ReflectionMethod extends ReflectionFunctionAbstract

    public function __construct(object|string $objectOrMethod, ?string $method = null) {}


According to RFC the last argument should be required

Is there follow-up for that? or passing one argument will remain til 8.4 as freeze in action?
https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#reflectionmethodconstruct

The RFC lists the last parameter as required only in one of the signatures, but since there is another one with 1 parameter, the stub - which unified the two signatures - displays the 2nd parameter as optional.

Is there follow-up for that?

What kind of follow-up do you mean? According to the RFC, the signatures and the behavior of ReflectionMethod::__construct() won't change until 9.0, except for the signature with 1 parameter will be deprecated in PHP 8.4. In 9.0, the constructor will only accept 2 arguments.

Thank you! misread

deprecate the second constructor signature in PHP 8.4

kylekatarnls · 2023-07-23T13:41:51Z

Hello, this introduced:

ErrorException: Calling ReflectionProperty::setValue() with a single argument is deprecated

But no alternative is given, and documentation still promote this way to set static properties:
https://www.php.net/manual/en/reflectionproperty.setvalue.php

So what to do in this particular case, and more generally, how do you expect developers to find out the fixes when hitting those kind of deprecations?

[EDIT]

Found setStaticPropertyValue, I'm testing now if it does the job. Sound like a documentation update is needed to replace the ->getProperty('foo')->setValue('bar') examples.

kocsismate · 2023-07-24T06:09:57Z

Hi @kylekatarnls ,

Thanks for the investigation! The RFC suggests another alternative: the the relevant section in https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures

Girgias · 2023-07-24T13:43:02Z

But no alternative is given, and documentation still promote this way to set static properties: https://www.php.net/manual/en/reflectionproperty.setvalue.php

So what to do in this particular case, and more generally, how do you expect developers to find out the fixes when hitting those kind of deprecations?

Migrations strategies will be written as part of the migration docs which are yet to be written.

…ded signatures](php/php-src#11703)

kocsismate added the RFC label Jul 13, 2023

kocsismate added this to the PHP 8.3 milestone Jul 13, 2023

kocsismate requested review from Girgias, bukka, derickr, devnexen, dstogov and iluuu1994 as code owners July 13, 2023 21:39

github-actions bot added Category: Engine Extension: intl Extension: date Extension: dba Extension: ffi Extension: ldap Extension: pgsql Extension: phar Extension: reflection Extension: standard labels Jul 13, 2023

Girgias reviewed Jul 14, 2023

View reviewed changes

kocsismate force-pushed the overridden-funcs branch from 3ebc7ea to ad1e175 Compare July 16, 2023 09:06

kocsismate force-pushed the overridden-funcs branch from ad1e175 to 37ff351 Compare July 16, 2023 10:02

Girgias reviewed Jul 16, 2023

View reviewed changes

ext/intl/tests/calendar_isSet_empty_time.phpt Outdated Show resolved Hide resolved

ext/pgsql/pgsql.c Show resolved Hide resolved

kocsismate force-pushed the overridden-funcs branch from 039945f to b85db30 Compare July 17, 2023 06:16

iluuu1994 reviewed Jul 17, 2023

View reviewed changes

kocsismate added 9 commits July 18, 2023 12:57

Add IntlCalendar::setDate() and IntlCalendar::setDateTime()

4cb7044

Implement IntlGregorianCalendar::createFromDate() and IntlGregorianCa…

b6bd874

…lendar::createFromDateTime()

Implement ldap_connect_wallet()

9eb703f

Implement ldap_exop_sync

d44beb3

Make the $row param of pg_fetch_result(), pg_field_prtlen() and pg_fi…

65b3c82

…eld_is_null() nullable

Deprecate Phar::setStub(resource $stub, int $length)

4ac47b9

Implement ReflectionMethod::createFromMethodName()

e0f2d15

Deprecate ReflectionProperty::setValue() with an incorrect 1st arg type

9f72c04

Implement stream_context_set_options()

08f8651

kocsismate force-pushed the overridden-funcs branch from a88a1ea to 08f8651 Compare July 18, 2023 10:58

kocsismate merged commit a5ad7e0 into php:master Jul 18, 2023

kocsismate deleted the overridden-funcs branch July 18, 2023 10:59

andypost reviewed Jul 19, 2023

View reviewed changes

kylekatarnls mentioned this pull request Jul 23, 2023

Document ReflectionProperty::setValue PHP 8.3 deprecation php/doc-en#2594

Merged

This was referenced Aug 10, 2023

PHP 8.3 | ✨ New PHPCompatibility.ParameterValues.RemovedGetClassNoArgs sniff (RFC) PHPCompatibility/PHPCompatibility#1614

Merged

PHP 8.3 | NewFunctions: recognize ldap_connect_wallet() and ldap_exop_sync() (RFC) PHPCompatibility/PHPCompatibility#1626

Merged

jrfnl mentioned this pull request Aug 17, 2023

PHP 8.3 | NewFunctions: recognize stream_context_set_options() (RFC) PHPCompatibility/PHPCompatibility#1631

Merged

llaville added a commit to llaville/php-compatinfo-db that referenced this pull request Oct 9, 2023

[PHP 8.3 support](#133) : implement [Deprecate functions with overloa…

a56dac2

…ded signatures](php/php-src#11703)

llaville added a commit to llaville/php-compatinfo-db that referenced this pull request Nov 18, 2023

[PHP 8.3 support](#133) : implement [Deprecate functions with overloa…

0079048

…ded signatures](php/php-src#11703)

llaville added a commit to llaville/php-compatinfo-db that referenced this pull request Nov 23, 2023

[PHP 8.3 support](#133) : implement [Deprecate functions with overloa…

fe58dfb

…ded signatures](php/php-src#11703)

ju1ius mentioned this pull request Dec 3, 2023

Signature of ReflectionProperty::setValue is incorrect as of PHP 8.3 php/doc-en#2987

Closed

		zend_error(E_DEPRECATED, "Calling dba_fetch() with $dba at the 3rd parameter is deprecated",
		ZSTR_VAL(fbc->common.function_name));

		@@ -205,6 +205,8 @@ class ReflectionMethod extends ReflectionFunctionAbstract

		public function __construct(object\|string $objectOrMethod, ?string $method = null) {}

Conversation

kocsismate commented Jul 13, 2023

Uh oh!

iluuu1994 commented Jul 13, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

kocsismate Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented Jul 16, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Girgias commented Jul 16, 2023

Uh oh!

kocsismate commented Jul 16, 2023

Uh oh!

Girgias commented Jul 16, 2023

Uh oh!

kocsismate commented Jul 17, 2023

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

iluuu1994 Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iluuu1994 Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andypost Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

andypost Jul 19, 2023 • 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!

kocsismate Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

andypost Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

kylekatarnls commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kocsismate commented Jul 24, 2023

Uh oh!

Girgias commented Jul 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

andypost Jul 19, 2023 •

edited

Loading

kylekatarnls commented Jul 23, 2023 •

edited

Loading