[RFC] [Discussion] Deprecate functions with overloaded signatures

Kamil Tekiela

3 years ago

Hi Máté,

I agree with this proposal, and I will be voting yes. The function
overloads are not a big issue for PHP developers, and some are very
much in use, but the reasons you listed are convincing and the manner
in which you propose to do it should create an easy upgrade path.

I think one year of deprecation is not enough. I believe the functions
that get replacements should be deprecated immediately in PHP 8.3 to
give people two years of deprecation notice. It doesn't make sense to
me to add a replacement but not deprecate the old variant.

`($rm = new ReflectionMethod(explode(“::”, $string));) ` I think you
forgot a splat operator here.

ReflectionProperty::setValue could have an alternative called
ReflectionProperty::setStaticValue. Then there would be no need for
the unused parameter anymore.

The overload of array_keys should be replaced with a new function
array_search_all. This overload is little known and very confusing.

The overload of ldap_exop should just be deprecated. An alternative
already exists and is the preferred way.

I wasn't aware that pg_execute has an overload apart from the default
connection one. Could you explain what that overload is?

stream_context_set_option should get alternative
stream_context_set_option_array.

What is the last item in Future scope supposed to be, because I think
ReflectionProperty is a typo.

Why is implode() not mentioned in this RFC?

Regards,
Kamil

Máté Kocsis

3 years ago

Hi Kamil,

Thank you for your reply, it is pretty much appreciated!

I think one year of deprecation is not enough. I believe the functions

> that get replacements should be deprecated immediately in PHP 8.3 to
> give people two years of deprecation notice. It doesn't make sense to
> me to add a replacement but not deprecate the old variant.
>

Yes, this upgrade path also makes sense to me, and I'm happy to go with
it if others don't disagree! Probably my approach would be advantageous
if we were earlier in the 8.x lifecycle.

> `($rm = new ReflectionMethod(explode(“::”, $string));) ` I think you
> forgot a splat operator here.
>

Fixed!

> ReflectionProperty::setValue could have an alternative called
> ReflectionProperty::setStaticValue. Then there would be no need for
> the unused parameter anymore.
>

Originally, my proposal had this method, but at last I removed it based on
Nicolas' suggestions. I think both ways make sense, a single setValue() is
probably much easier from the migration point of view, since the suggested
alternative is already there in older PHP versions. And it doesn't mean
we cannot have a ReflectionProperty::setStaticValue() later on. :)

> The overload of array_keys should be replaced with a new function
> array_search_all. This overload is little known and very confusing.
>

Thanks for pointing this out, I'll look into this soon!

>
> The overload of ldap_exop should just be deprecated. An alternative
> already exists and is the preferred way.
>

I've just included ldap_exop() in the RFC, albeit slightly differently to
your
suggestion: since this function performs the extended operation
synchronously
or asynchronously based on whether the $response_data parameter is provided,
we cannot simply deprecate and remove the latter one, because as far as I
can understand, synchronous communication is also a valid use-case. So I
went
with adding an ldap_exop_sync() function.

> I wasn't aware that pg_execute has an overload apart from the default
> connection one. Could you explain what that overload is?
>

You are right, as it turned out for me, there is no other overload indeed,
so I removed
it from the RFC.

> stream_context_set_option should get alternative
> stream_context_set_option_array.
>

Thanks for the suggestion! I incorporated this suggestion into the RFC
(using
the stream_context_set_options name though).

What is the last item in Future scope supposed to be, because I think

> ReflectionProperty is a typo.
>

Right, it's now removed.

> Why is implode() not mentioned in this RFC?
>

Because it's not overloaded anymore. :) Even though the manual lists
multiple signatures
for it, its exact signature is the following:

function implode(string|array $separator, ?array $array = null): string {}

I'm not sure how much people would miss the possibility to use implode with
a
single argument ("implode($array)"), but if that's the case, we can add it
to the
usual PHP 8.3 or PHP 8.4 Deprecations RFC. I wouldn't like to add it to
mine,
because it's already way too long.

Regards:
Máté

Dan Ackroyd

3 years ago

Kamil Tekiela wrote:

> > I think one year of deprecation is not enough... It doesn't make
> > sense to me to add a replacement but not deprecate the old variant.

Máté Kocsis <kocsismate90@gmail.com> wrote:

> Yes, this upgrade path also makes sense to me, and I'm happy to go with
> it if others don't disagree!

For the functions that are having new separate methods added, not
deprecating them immediately makes upgrading easier. When upgrading
from one version of PHP to the next, it is best if you can run the
same code on both versions, without any deprecation notices going off.

Would leaving the current versions in place and working actually cause
any maintenance issues?

If not, then we could just move really slowly on deprecating them. We
could deprecate them at some point in the 9.x release cycle and remove
them in 10.0.

The RFC says:

> Impact analysis: 1 out of the 2000 most popular PHP packages rely on calling session_set_save_handler() with 6 or more arguments.

I doubt analysing github is going to give a useful measure of the
impact of this RFC. Functions like session_set_save_handler are going
to be used in custom code written for a company than in shared
libraries.

cheers
Dan
Ack

Tim Düsterhus

3 years ago

Hi

On 5/2/23 13:37, Dan Ackroyd wrote:

> For the functions that are having new separate methods added, not
> deprecating them immediately makes upgrading easier. When upgrading
> from one version of PHP to the next, it is best if you can run the
> same code on both versions, without any deprecation notices going off.

Agree here, I consider it a good thing if there's at least one version
where both alternatives exist and are fully supported (i.e. without
emitting a deprecation notice).

As an example, with PHP 8.3's addition of Randomizer::getFloat() and
Randomizer::getBytesFromString() there are useful replacements for
lcg_value() and uniqid() [1] respectively, but I'm intentionally not
proposing deprecating either of them before whatever comes after PHP 8.3.

[1] The latter of which is likely one of the most misused functions in PHP.

> Would leaving the current versions in place and working actually cause
> any maintenance issues?
>
> If not, then we could just move really slowly on deprecating them. We
> could deprecate them at some point in the 9.x release cycle and remove
> them in 10.0.

Does it necessarily follow that a function that is deprecated in major X
must be removed in major X+1? Otherwise deprecating with 8.3 and
removing in 10 would be an option that could be evaluated when it's time
for 9 and the migration did not yet "sufficiently" happen in userland code.

I can also see how that would be useful for the planned deprecation of
rand() and mt_rand(). Both of them have overloaded signatures, but will
be part of the other deprecation RFC, as they also have a myriad of
other issues that I consider to be worse than the signature.
Unfortunately they are pretty ubiquitous, but luckily do not cause
"internal" maintenance problems. So it would likely be feasible to keep
them until 10, as long as they emit the deprecation notice to slowly nag
developers away from them towards better alternatives.

Best regards
Tim Düsterhus

Larry Garfield

3 years ago

On Tue, May 2, 2023, at 5:05 PM, Tim Düsterhus wrote:

> Hi
>
> On 5/2/23 13:37, Dan Ackroyd wrote:
>> For the functions that are having new separate methods added, not
>> deprecating them immediately makes upgrading easier. When upgrading
>> from one version of PHP to the next, it is best if you can run the
>> same code on both versions, without any deprecation notices going off.
>
> Agree here, I consider it a good thing if there's at least one version
> where both alternatives exist and are fully supported (i.e. without
> emitting a deprecation notice).
>
> As an example, with PHP 8.3's addition of Randomizer::getFloat() and
> Randomizer::getBytesFromString() there are useful replacements for
> lcg_value() and uniqid() [1] respectively, but I'm intentionally not
> proposing deprecating either of them before whatever comes after PHP 8.3.
>
> [1] The latter of which is likely one of the most misused functions in PHP.
>
>> Would leaving the current versions in place and working actually cause
>> any maintenance issues?
>>
>> If not, then we could just move really slowly on deprecating them. We
>> could deprecate them at some point in the 9.x release cycle and remove
>> them in 10.0.
>
> Does it necessarily follow that a function that is deprecated in major X
> must be removed in major X+1? Otherwise deprecating with 8.3 and
> removing in 10 would be an option that could be evaluated when it's time
> for 9 and the migration did not yet "sufficiently" happen in userland code.
>
> I can also see how that would be useful for the planned deprecation of
> rand() and mt_rand(). Both of them have overloaded signatures, but will
> be part of the other deprecation RFC, as they also have a myriad of
> other issues that I consider to be worse than the signature.
> Unfortunately they are pretty ubiquitous, but luckily do not cause
> "internal" maintenance problems. So it would likely be feasible to keep
> them until 10, as long as they emit the deprecation notice to slowly nag
> developers away from them towards better alternatives.
>
> Best regards
> Tim Düsterhus

Given past pushback on changes that we've gotten, a low-cost/no-cost long deprecation period seems like it would be very appreciated.

I've actually been considering proposing an RFC to mandate a minimum time length between deprecation and actual removal/change for that reason.

--Larry Garfield

Rowan Collins

3 years ago

On 27/04/2023 22:28, Máté Kocsis wrote:

> As you have possibly already experienced, overloaded signatures cause
> various smaller and bigger issues, while the concept is not natively
> supported by PHP. That's why I drafted an RFC which intends to phase out
> the majority of overloaded function/method signatures and also forbid
> the introduction of such functions in the future:
> https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures

Hi Máté,

While I broadly agree with the sentiment behind this, I think the
upgrade path needs to be examined a bit more closely.

As Kamil says, a potential 1-year deprecation is probably not enough,
and we need to keep in mind that many libraries and applications need to
support multiple versions of PHP at once. If they are to become errors
in 9.0, there should be some way to write code that will run on both 8.0
and 9.0.

The only ones in this list I can see that being easy for are
assert_options (you can write a polyfill for the new assert_set_option,
and switch all code to use that), and get_class() / get_parent_class()
(you can start passing $this right away).

A lot of the others introduce new methods, which can't easily
polyfilled, or directly change the signature. For all of those, users
would have to write an additional wrapper, with explicit detection of
the PHP version to make the call in one form or the other.


I have particular concerns about session_set_save_handler. The impact is
hard to estimate, because it will be used directly in applications more
often than in libraries. The multiple parameter signature is much older,
dating back to PHP 4, with the object form added only in PHP 5.4. That
may seem a long time ago, but there is currently no incentive to change
existing code between the styles, and doing so requires a reasonable
amount of work.


On a different point, I think "assert_options" is a peculiar name for
either setting or getting a single option, and would suggest it be
replaced with two new functions, assert_get_option and
assert_set_option. Replacing both variants also makes it easier for
users to find everywhere they've used it, and polyfill both variants,
rather than having to examine each.


Regards,

--
Rowan Tommins
[IMSoP]

Rowan Collins

3 years ago

On Fri, 28 Apr 2023 at 09:16, Rowan Tommins <rowan.collins@gmail.com> wrote:

> On a different point, I think "assert_options" is a peculiar name for
> either setting or getting a single option, and would suggest it be
> replaced with two new functions, assert_get_option and
> assert_set_option.
>

Further to this, I've realised that the manual is currently a bit confusing
regarding the interaction of ini_set('zend.assertions', ...),
ini_set('assert.active', ...), and assert_options(ASSERT_ACTIVE, ...)

Either way, all the options to assert_options are documented as having an
equivalent ini setting, so is this function actually needed at all, or
should users be encouraged to use ini_get / ini_set directly?

https://www.php.net/assert_options

Regards,

--
Rowan Tommins
[IMSoP]

Máté Kocsis

3 years ago

Hi Rowan,

As Kamil says, a potential 1-year deprecation is probably not enough,

> and we need to keep in mind that many libraries and applications need to
> support multiple versions of PHP at once. If they are to become errors
> in 9.0, there should be some way to write code that will run on both 8.0
> and 9.0.
>

Yes, I agree that we should make the upgrade path of libraries as easy as
possible,
and I also agree that probably a 1-year deprecation is not enough.. although
the suggested alternatives would be available 2 years in advance of the
removal
so this would allow a 2 year window for fixing code using the deprecated
functionality.

> The only ones in this list I can see that being easy for are
> assert_options (you can write a polyfill for the new assert_set_option,
> and switch all code to use that), and get_class() / get_parent_class()
> (you can start passing $this right away).
>

As far as I see there are more deprecations which are easy to fix. In order
to see the whole picture
better, let's group the functions based on how a possible upgrade path
would look like:

- With existing suggested alternative:
- get_class() and get_parent_class()
- pg_fetch_result(), pg_field_prtlen(), and pg_field_is_null()
- Phar::setStub()
- ReflectionMethod::__construct()
- ReflectionProperty::setValue()
- session_set_save_handler() (because the OO and procedural style are
interchangeable)
- stream_context_set_option()
- Polyfillable:
- assert_options()
- ldap_connect()
- ldap_exop()
- Requires conditional version check:
- DatePeriod::__construct()
- IntlCalendar::set()
- IntlGregorianCalendar::__construct()

(I intentionally did not include the FFI related methods, since this
section is TBD)

As it can be seen, most deprecations have an existing alternative. The 2nd
category (polyfillable ones)
mostly contains very little used functions, especially because the
deprecation of ldap_connect() only
affects PHP compiled with OracleLDAP.

The category which requires conditional PHP version checks is indeed the
most problematic one.
Probably it's not a surprise that it only includes methods because it's not
possible to polyfill methods
according to my knowledge. The good news is that IntlCalendar::set() and
IntlGregorianCalendar::__construct()
seem to be very little used functionality. So in my opinion,
DatePeriod::__construct() has the worst upgrade path,
the rest are either easy or shouldn't cause much issues. Since the
constructor is set to be removed completely
based on Derick's preference, we should discuss with him whether it's worth
to leave the
public function __construct(DateTimeInterface $start, DateInterval
$interval, DateTimeInterface $end, int $options = 0) {}
signature alive so that people can change their code to use this instead of
the other two signatures.

I have particular concerns about session_set_save_handler. The impact is

> hard to estimate, because it will be used directly in applications more
> often than in libraries. The multiple parameter signature is much older,
> dating back to PHP 4, with the object form added only in PHP 5.4. That
> may seem a long time ago, but there is currently no incentive to change
> existing code between the styles, and doing so requires a reasonable
> amount of work.
>

Yes, but changing session_set_save_handler() to session_set_save_handlers()
should be a reasonably small effort, isn't it? I understand that it's going
to affect
lots of codebases, however other PHP 8.x deprecations are much more
difficult
to fix than this one would be.

> On a different point, I think "assert_options" is a peculiar name for
> either setting or getting a single option, and would suggest it be
> replaced with two new functions, assert_get_option and
> assert_set_option. Replacing both variants also makes it easier for
> users to find everywhere they've used it, and polyfill both variants,
> rather than having to examine each.
>

Yes, I agree that the assert_options() name is at least weird but I
wouldn't like to
include changes into this RFC which are not strictly related to overloaded
signatures. Just like in case of implode(), the 1-parameter version of
assert_options()
could be added to the PHP 8.3/8.4 deprecations RFC though.

Regards,
Máté

Rowan Collins

3 years ago

On Tue, 2 May 2023 at 13:20, Máté Kocsis <kocsismate90@gmail.com> wrote:

> Yes, but changing session_set_save_handler() to session_set_save_handlers()
> should be a reasonably small effort, isn't it? I understand that it's
> going to affect
> lots of codebases, however other PHP 8.x deprecations are much more
> difficult
> to fix than this one would be.
>

Oh, did that proposal change, or did I just misread it?

Maybe because the function names are so similar? In general, I hate
variable and function names that differ only by an "s", it's far too easy
to misread or mistype them. Maybe "session_set_save_handler_functions"?

For the same reason, I much prefer Kamil's suggestion of "
stream_context_set_option_array" over " stream_context_set_options".


As an aside, I don't think "other deprecations are already more difficult"
is a good argument - it's like saying "yes, I punched him, but not as hard
as someone else already had". I think the change can be defended from other
angles, but wanted to call that out.

> Yes, I agree that the assert_options() name is at least weird but I
> wouldn't like to
> include changes into this RFC which are not strictly related to overloaded
> signatures. Just like in case of implode(), the 1-parameter version of
> assert_options()
> could be added to the PHP 8.3/8.4 deprecations RFC though.
>

It *is* strictly related, though: the current function has two purposes:
get an option, and set an option; the RFC proposes to split that into two
functions, and there are three ways we can do that:

1) Keep the current name for get, come up with a new name for set
2) Come up with a new name for get, keep the current name for set
3) Come up with new names for both get and set

I then looked further, and suggested:

4) Deprecate the existing function, but do not create any new functions;
instead, recommend ini_get for get, and ini_set for set

All four options are direct remedies to the overloaded signature, and I
think due to the current unclear naming, options 3 and 4 are superior to
options 1 and 2. Do you have a specific reason to prefer option 1?


Regards,

--
Rowan Tommins
[IMSoP]

Unnamed Person

3 years ago

On Tuesday 02 May 2023 15:07:21 (+02:00), Rowan Tommins wrote:

> On Tue, 2 May 2023 at 13:20, Máté Kocsis  wrote:
>
> > Yes, I agree that the assert_options() name is at least weird but I
> > wouldn't like to
> > include changes into this RFC which are not strictly related to overloaded
> > signatures. Just like in case of implode(), the 1-parameter version of
> > assert_options()
> > could be added to the PHP 8.3/8.4 deprecations RFC though.
> >
>
>
> It *is* strictly related, though: the current function has two purposes:
> get an option, and set an option; the RFC proposes to split that into two
> functions, and there are three ways we can do that:
>
> 1) Keep the current name for get, come up with a new name for set
> 2) Come up with a new name for get, keep the current name for set
> 3) Come up with new names for both get and set
>
> I then looked further, and suggested:
>
> 4) Deprecate the existing function, but do not create any new functions;
> instead, recommend ini_get for get, and ini_set for set
>
> All four options are direct remedies to the overloaded signature, and I
> think due to the current unclear naming, options 3 and 4 are superior to
> options 1 and 2. Do you have a specific reason to prefer option 1?

I can't answer that question but checked especially option 4) as it looked promising to me.

In the overall context, let's not forget this note [1]:

> While assert_options() can still be used to control behaviour as described above for backward compatibility reasons, PHP 7 only code should use the two new configuration directives to control the behaviour of assert() and not call assert_options().

That is zend.assertions and assert.exception only (of which only the second has a counterpart in ASSERT_EXCEPTION as the first can not be set at runtime).

That means: Before investing too much thought which of all the options, IMHO the general route should be to deprecate assert_options() overall to prepare its removal (and with it a good share of the assert.options).

From signature / types perspective, ASSERT_CALLBACK option is not 100% compatible w/ ini_get()/ini_set() as they don't support all types of the callback pseudotype. but have not double checked this as I ran over the above mentioned remark which I believe should be leading (the deprecation of the pre PHP 7.0 assert/options alltogether).

With the PHP 8.0 release this got some traction (upgrade of the assert callback signature and ASSERT_QUIET_EVAL option seems to be gone (perhaps in alignment with the improvements in the error handler callback signature on the level parameter that is now the error type regardless of error_level())).

So perhaps this for the current RFC:

4) Deprecate the existing function, but do not create any new functions; instead, recommend the two configuration settings zend.assertions and assert.exception only, for those that can be set at runtime recommend ini_get()/ini_set(), for callback further investigate (does it still work?), for the moment I see no alternative to assert_options() for callback values not of type null or string (e.g. callabe array or callable object) as those are not supported by ini_get()/in_set().

-- hakre

p.s. thank you for creating this RFC and taking care.

[1]: https://www.php.net/manual/en/function.assert.php

Máté Kocsis

3 years ago

Hi Rowan,

> As an aside, I don't think "other deprecations are already more difficult"
> is a good argument - it's like saying "yes, I punched him, but not as hard
> as someone else already had". I think the change can be defended from other
> angles, but wanted to call that out.
>

I admit that my argument was not a good one, but I intended to mean that
most of
the suggested alternatives are not too difficult to implement: since
numerous other proposals
were accepted in the past with a much more difficult upgrade path, I think
this aspect
of my RFC shouldn't be a deal breaker considering the benefits.

Maybe because the function names are so similar? In general, I hate

> variable and function names that differ only by an "s", it's far too easy
> to misread or mistype them. Maybe "session_set_save_handler_functions"?

To be honest, I'm not a fan of using the longer names
(session_set_save_handler_functions
and stream_context_set_option_array), especially the latter one looks weird
to me, and it
doesn't go well with stream_context_get_options().

Besides, a most interesting development is that I've just discovered the
stream_context_get_params()
and stream_context_set_params() functions which do nearly the same what the
stream_context_*et_option
functions do: the only difference between them is that the latter functions
wrap the options inside the
"options" key and also has an additional "notification" key.

It *is* strictly related, though: the current function has two purposes:

> get an option, and set an option; the RFC proposes to split that into two
> functions, and there are three ways we can do that:

Thanks for the idea, your explanation made sense to me, so finally, I
removed assert_options() from
my proposal and the deprecation of assert_options() relies on the PHP 8.3
deprecations RFC.

Regards:
Máté

Larry Garfield

3 years ago

On Tue, May 2, 2023, at 12:20 PM, Máté Kocsis wrote:

> As far as I see there are more deprecations which are easy to fix. In order
> to see the whole picture
> better, let's group the functions based on how a possible upgrade path
> would look like:
>
> - With existing suggested alternative:
>     - get_class() and get_parent_class()
>     - pg_fetch_result(), pg_field_prtlen(), and pg_field_is_null()
>     - Phar::setStub()
>     - ReflectionMethod::__construct()
>     - ReflectionProperty::setValue()
>     - session_set_save_handler() (because the OO and procedural style are
> interchangeable)
>     - stream_context_set_option()
> - Polyfillable:
>     - assert_options()
>     - ldap_connect()
>     - ldap_exop()
> - Requires conditional version check:
>     - DatePeriod::__construct()
>     - IntlCalendar::set()
>     - IntlGregorianCalendar::__construct()
>
> (I intentionally did not include the FFI related methods, since this
> section is TBD)
>
> As it can be seen, most deprecations have an existing alternative. The 2nd
> category (polyfillable ones)
> mostly contains very little used functions, especially because the
> deprecation of ldap_connect() only
> affects PHP compiled with OracleLDAP.

Thanks for the breakdown.  I am also generally in favor of the changes here, but am concerned about the migration path.  The key factor, IMO, is that it should be possible to run code without version checks across as wide a range of versions as feasible.  Needing version checks to run from 8.2 to 9.0 is going to be problematic.  Were we earlier in the 8.x cycle it would be less of an issue, but assuming we stick to the usual schedule it feels too tight on those.

As Rowan asked, how much would it hurt to leave the date-related methods available but deprecated until 10.x?

I'm also not entirely clear how you'd polyfill functions that exist, but change behavior.

--Larry Garfield

Máté Kocsis

3 years ago

Hi Dan (and Larry and Tim who responded to the same topic),

Would leaving the current versions in place and working actually cause

> any maintenance issues?

Yes, unfortunately they cause some maintenance issues: since they don't
have a single
signature, gen_stub.php generates the wrong signature for them in the
manual. For example
https://www.php.net/manual/en/function.stream-context-set-option.php
displays two distinct
signatures, however gen_stub.php only knows one signature, so obviously
generation won't work well:

function stream_context_set_option($context, array|string
$wrapper_or_options, ?string $option_name = null, mixed $value = UNKNOWN):
bool {}

I use gen_stub.php for automatically detecting the outdated
function/method/class signatures which I can then
commit with minimal effort. Actually my doc-en PRs (
https://github.com/php/doc-en/pulls?page=1&q=is%3Apr+is%3Aclosed+author%3Akocsismate
)
are either fixing outdated information in the manual, or ensuring that
gen_stub.php can generate the same
information that we display in the docs. Now, there are only ~50-60
function/method signatures
in master which are not in line  with the output of gen_stub.php, and most
of them are due to
overloaded signatures (the rest is due to PHP 8.3 changes which shouldn't
be documented yet).
For reference, the number of outdated function signatures used to be many
hundreds or probably 1000+
when the project started, just before PHP 8.0 came out.

If not, then we could just move really slowly on deprecating them. We

> could deprecate them at some point in the 9.x release cycle and remove
> them in 10.0.

Although I would love to get rid of as many overloaded signatures as
possible due to the above mentioned fact,
I admit that there might be some signatures which other internals would
prefer to phase out slower than me. So
thanks for the idea! Now, the individual votes in the RFC contain 2 options
for removing a signature: a shorter path (deprecation
in PHP 8.4, removal in PHP 9.0) and a longer one (deprecation in PHP 9.0
and removal in PHP 10.0).

Additionally, I wrote a very explicit and detailed list about the suggested
BC compatible alternatives for each deprecation so that
now it should be very clear how difficult it is to replace a
function/method invocation.

The RFC says:

Impact analysis: 1 out of the 2000 most popular PHP packages rely on

> calling session_set_save_handler() with 6 or more arguments.

> I doubt analysing github is going to give a useful measure of the
> impact of this RFC. Functions like session_set_save_handler are going
> to be used in custom code written for a company than in shared
> libraries.

Yes, I'm aware of this glitch, but that's the best I could provide for
estimating the impact of the deprecations.

Cheers,
Máté

Tim Düsterhus

3 years ago

Hi

On 5/11/23 00:10, Máté Kocsis wrote:

> If not, then we could just move really slowly on deprecating them. We
>> could deprecate them at some point in the 9.x release cycle and remove
>> them in 10.0.
>
>
> Although I would love to get rid of as many overloaded signatures as
> possible due to the above mentioned fact,
> I admit that there might be some signatures which other internals would
> prefer to phase out slower than me. So
> thanks for the idea! Now, the individual votes in the RFC contain 2 options
> for removing a signature: a shorter path (deprecation
> in PHP 8.4, removal in PHP 9.0) and a longer one (deprecation in PHP 9.0
> and removal in PHP 10.0).
>

I believe this vote format ("three options") is not really compatible
with the voting rules (https://wiki.php.net/rfc/voting).

For example it's not entirely clear what would happen here:

5 votes to deprecate in 8.3 / remove 9.0
4 votes to deprecate in 9.0 / remove 10.0
4 votes to not deprecate.

Neither option has a 2/3 majority over any other option. The two
deprecation options together have a 2/3 majority over not deprecating,
which might indicate that a deprecation should happen. But in which
version? This would likely require ranked choice voting which feels
complex for something that should be a simple vote.

I've had a similar issue with my "Improve unserialize() error handling
RFC" (https://externals.io/message/118566) and opted to make an
executive decision as the RFC author and not offer one of the possible
version targets to avoid that problem entirely.

----------------------

Repeating my previous question:

> Does it necessarily follow that a function that is deprecated in major X
> must be removed in major X+1? Otherwise deprecating with 8.3 and
> removing in 10 would be an option that could be evaluated when it's time
> for 9 and the migration did not yet "sufficiently" happen in userland code.

Would it be an option to just deprecate everything in PHP 8.3 and defer
the vote for the removal target version to a later date? This would give
users the deprecation notice as early as possible and also gives them
appropriate time to migrate if migration for a specific set of functions
is (much) slower than others. It doesn't really feel right to decide on
a deprecation for 9.0 / removal for 10.0 at the current time, but
deprecation in 8.3 + removal when it's ready seems to be okay.

Best regards
Tim Düsterhus

Dan Ackroyd

3 years ago

On Thu, 11 May 2023 at 10:36, Tim Düsterhus <tim@bastelstu.be> wrote:

>
> Would it be an option to just deprecate everything in PHP 8.3

As I said before, having at least one version where the new versions
of the functions are available, and the old versions aren't giving
deprecation notices, is the 'cleanest' way of refactoring functions.

Deprecation notices only deliver their value when the old versions of
the functions are removed, and before then are a cost.

> It doesn't really feel right to decide on
> a deprecation for 9.0 / removal for 10.0 at the current time, but
> deprecation in 8.3 + removal when it's ready seems to be okay.

I think I don't understand your concern.

If a problem is discovered with a planned removal of something, people
on this project are mostly reasonable*, and a small RFC to adjust to
the new circumstances would be very likely to pass.

cheers
Dan
Ack

* though sometimes they base their decisions on experiences/value
different from my own, which makes it not feel like that.

Tim Düsterhus

3 years ago

Hi

On 5/11/23 18:58, Dan Ackroyd wrote:

> On Thu, 11 May 2023 at 10:36, Tim Düsterhus <tim@bastelstu.be> wrote:
>>
>> Would it be an option to just deprecate everything in PHP 8.3
>
> As I said before, having at least one version where the new versions
> of the functions are available, and the old versions aren't giving
> deprecation notices, is the 'cleanest' way of refactoring functions.

Right. Replace "8.3" by "the first version after the replacement is
available" in my previous email. My main question was …

> Deprecation notices only deliver their value when the old versions of
> the functions are removed, and before then are a cost.
>
>> It doesn't really feel right to decide on
>> a deprecation for 9.0 / removal for 10.0 at the current time, but
>> deprecation in 8.3 + removal when it's ready seems to be okay.
>
> I think I don't understand your concern.
>
> If a problem is discovered with a planned removal of something, people
> on this project are mostly reasonable*, and a small RFC to adjust to
> the new circumstances would be very likely to pass.

… the following: I wanted to know if

"Deprecate in 8.x + not remove in 9.0, but only remove in 10.x or 11.x"

would be an option, because I feel that it is more useful than

"If we deprecate in 8.x then we MUST remove in 9.0, that's why we're
already making deprecation plans for 9.x, so we are able to remove in 10.0"

All the discussion so far had "Deprecate in x.y and remove in x+1.0",
but not "Deprecate in x.y and remove in x+n.0 with n > 1".

Best regards
Tim Düsterhus

Dan Ackroyd

3 years ago

On Thu, 11 May 2023 at 10:36, Tim Düsterhus <tim@bastelstu.be> wrote:

>
> I believe this vote format ("three options") is not really compatible
> with the voting rules (https://wiki.php.net/rfc/voting).
>
> For example it's not entirely clear what would happen here:
>
> 5 votes to deprecate in 8.3 / remove 9.0
> 4 votes to deprecate in 9.0 / remove 10.0
> 4 votes to not deprecate.

The RFC author could just say that yes votes for deprecation and
eventual removal will be added together, with the timescale being a
preference vote.

I think the only people who would object to that are people who would
vote yes to "deprecate and remove" but only if it matches their
preferred timescale, and would otherwise vote no. Which probably isn't
a thing.

cheers
Dan
Ack

Máté Kocsis

3 years ago

Hi Dan and Tim,

> The RFC author could just say that yes votes for deprecation and
> eventual removal will be added together, with the timescale being a
> preference vote.
>

Thank you, Dan, for the  clarification, that was indeed what I meant. I'll
also
make this clear in the RFC, unfortunately I forgot it so far.

… the following: I wanted to know if

> "Deprecate in 8.x + not remove in 9.0, but only remove in 10.x or 11.x"

Yes, this is also a possible choice. Actually, I'm sure there were multiple
things deprecated in 5.x,
which only got removed in PHP 8.0. At least this is what I remember. :)

The reason why I still opted for the possibility of "deprecate in 9.0 and
remove in 10.0",
because in my opinion this choice would suit better some of the most
widespread functionalities.
Not deprecating them formally straightaway in 8.x versions, but only
providing a suggested
alternative instead of them, would first of all buy some time for people
who don't yet want to deal with this problem at all, but still allow them
to voluntarily migrate
when they are ready without nagging them. Then they had a relatively long
time period when we would
warn them a bit louder. A full major release cycle should be more than
enough time to perform the migration.

With all that said, personally I don't think any of the affected
functionalities (maybe with the exception
of only one of them) warrant such a long phasing out period.

Regards,
Máté

Rowan Collins

3 years ago

On 11 May 2023 18:06:44 BST, Dan Ackroyd <Danack@basereality.com> wrote:

>On Thu, 11 May 2023 at 10:36, Tim Düsterhus <tim@bastelstu.be> wrote:
>>
>> I believe this vote format ("three options") is not really compatible
>> with the voting rules (https://wiki.php.net/rfc/voting).
>>
>> For example it's not entirely clear what would happen here:
>>
>> 5 votes to deprecate in 8.3 / remove 9.0
>> 4 votes to deprecate in 9.0 / remove 10.0
>> 4 votes to not deprecate.
>
>The RFC author could just say that yes votes for deprecation and
>eventual removal will be added together, with the timescale being a
>preference vote.
>
>I think the only people who would object to that are people who would
>vote yes to "deprecate and remove" but only if it matches their
>preferred timescale, and would otherwise vote no. Which probably isn't
>a thing.

I think it's actually very likely that voters would want to express "deprecate, but do not remove before 10.0". Treating those votes as "generally in favour, so enough to approve removal in 9.0" doesn't seem appropriate.

The other way around - "deprecate, but do not remove later than 9.0" - seems less likely. I also don't see any reason to delay the deprecation - the whole point of the longer period is to give people more notice.

So I would suggest rewording the options slightly:

a) Deprecate in 8.3, remove in either 9.0 or 10.0
b) Deprecate in 8.3, remove in 10.0
c) Do not deprecate

Now if the votes are a:5, b:4, c:4, we can say:

- 9 people voted for deprecation in 8.3, vs 4 against
- only 5 voted for removal in 9.0, vs 8 against
- 9 voted for removal in 10.0, vs 4 against

So we conclude that we should deprecate in 8.3, and remove in 10.0


The suggestion to narrow it down to a yes/no proposal in the discussion phase is probably even better, though.

Regards,

--
Rowan Tommins
[IMSoP]

Larry Garfield

3 years ago

On Fri, May 12, 2023, at 6:52 AM, Rowan Tommins wrote:

> I think it's actually very likely that voters would want to express
> "deprecate, but do not remove before 10.0". Treating those votes as
> "generally in favour, so enough to approve removal in 9.0" doesn't seem
> appropriate.
>
> The other way around - "deprecate, but do not remove later than 9.0" -
> seems less likely. I also don't see any reason to delay the deprecation
> - the whole point of the longer period is to give people more notice.
>
> So I would suggest rewording the options slightly:
>
> a) Deprecate in 8.3, remove in either 9.0 or 10.0
> b) Deprecate in 8.3, remove in 10.0
> c) Do not deprecate
>
> Now if the votes are a:5, b:4, c:4, we can say:
>
> - 9 people voted for deprecation in 8.3, vs 4 against
> - only 5 voted for removal in 9.0, vs 8 against
> - 9 voted for removal in 10.0, vs 4 against
>
> So we conclude that we should deprecate in 8.3, and remove in 10.0
>
>
> The suggestion to narrow it down to a yes/no proposal in the discussion
> phase is probably even better, though.

Personally I'm on team "add new functions now, deprecate old ones in 9, remove old ones in 10."

1. As noted, it gives people a clean window in which to use the old or new ones without getting a deprecation warning, and without cutting off support for old but still supported PHP versions.
2. It gives people ample time to fix their code.  (Around 7-8 years.)
3. Perhaps most importantly, it's a good-faith gesture to the people who have objected (validly, if sometimes impolitely and crudely) to shorter deprecation periods in the past.  There is definitely a relationship building to be done with the broader user-space community, and offering a very wide grace period is a good way to help with that.

That these particular functions and uses are quite rare doesn't really change any of that; if anything it strengthens it, that we're willing to be graceful about it even in cases where we could probably get away with not being so.

--Larry Garfield

Máté Kocsis

3 years ago

>
> So I would suggest rewording the options slightly:
>
> a) Deprecate in 8.3, remove in either 9.0 or 10.0
> b) Deprecate in 8.3, remove in 10.0
> c) Do not deprecate
>
> Now if the votes are a:5, b:4, c:4, we can say:
>
> - 9 people voted for deprecation in 8.3, vs 4 against
> - only 5 voted for removal in 9.0, vs 8 against
> - 9 voted for removal in 10.0, vs 4 against
>
> So we conclude that we should deprecate in 8.3, and remove in 10.0
>

While this vote format looks a bit confusing for me at the first sight, I'm
OK to apply it. Although, I don't fully agree with
the "deprecate in 8.3 and remove in 10.0" part due to the reason I wrote
about in my previous email.

In my opinion, if we want to keep a signature for around 5-7 more years
then we should really wait ~2 years with the
deprecation so that people can react voluntarily first. Libraries have to
get rid of deprecated calls in any case, otherwise
their users will surely start to complain. If they do fix these calls in
their PHP 8.x compatible code then there's much less reason
to postpone the removal with 5 more years.

The suggestion to narrow it down to a yes/no proposal in the discussion

> phase is probably even better, though.
>

I agree with this, but so far there was no feedback which functions/methods
people want to deprecate slower or faster,
so I'm eager to hear about your and everyone else's opinion!

Replying to Larry:

That these particular functions and uses are quite rare doesn't really

> change any of that; if anything it strengthens it, that we're willing to be
> graceful about it even in cases where we could probably get away with not
> being so.

I don't think this "generosity" is really necessary, even though I
appreciate your intention. For example,
in case of ldap_connect(), the signature to be deprecated is not even
documented at php.net... and it's only available on
a very specific platform. I see no reason to keep this signature for 5+
years. The same goes for IntlGregorianCalendar::__construct()
which seems to be a very underrepresented class as well.

As I already said, most of the functionality to be removed is easy to
replace, and it could be done via automation:
i.e. you add Symfony Polyfill to your project and then Rector swaps the
problematic function calls to the good ones. Sometimes
not even tooling is needed, a simple search + replace is enough. With all
that said, I think only a smaller subset
of functions may deserve the longer depreciation period.

Máté

Rowan Collins

3 years ago

On 12 May 2023 19:17:20 BST, "Máté Kocsis" <kocsismate90@gmail.com> wrote:

>Libraries have to
>get rid of deprecated calls in any case, otherwise
>their users will surely start to complain.

I've said it before, and I'll say it again: the solution to this is not fewer deprecation messages; it's better documentation to stop people confusing deprecations for errors, and better functionality for users to choose which messages they see.

--
Rowan Tommins
[IMSoP]

Dan Ackroyd

3 years ago

From the RFC:

>
> Code which invokes get_class() without parameters should be modified to
> use __CLASS__ or get_class($this) instead,

I don't think the first option is equivalent:

class C {
function printType() {
echo "__CLASS__ is " . __CLASS__ . "\n";
echo "get_class is " . get_class($this) . "\n";
}
}

class D extends C {
function printTypeInChild() {
echo "__CLASS__ is " . __CLASS__ . "\n";
echo "get_class is " . get_class($this) . "\n";
}
}

(new D)->printType();
(new D)->printTypeInChild();

// Output is:
__CLASS__ is C
get_class is D
__CLASS__ is D
get_class is D

And unless I'm missing something, the second option doesn't appear
usable with static methods, which is also a problem with
get_parent_class()

> while code which invokes
> get_parent_class() without parameters should be modified to use
> get_parent_class($this) instead.

What would the equivalent code get_parent_class() for static methods? e.g.

class A {}

class B extends A {
public static function foo() {
echo get_parent_class() . " \n";
}
}
b::foo();

cheers
Dan
Ack

Tim Düsterhus

3 years ago

Hi

On 5/2/23 13:56, Dan Ackroyd wrote:

> And unless I'm missing something, the second option doesn't appear
> usable with static methods, which is also a problem with
> get_parent_class()

static::class or self::class appear to do the right thing depending on
what exactly you want to retrieve.

>> while code which invokes
>> get_parent_class() without parameters should be modified to use
>> get_parent_class($this) instead.
>
> What would the equivalent code get_parent_class() for static methods? e.g.
>

parent::class appears to do the right thing.

Best regards
Tim Düsterhus

A.L.E.C

3 years ago

On 27.04.2023 23:28, Máté Kocsis wrote:

> https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures

stream_context_set_option() should be removed from the Future Scope as
it already is being changed by the RFC.

--
Aleksander Machniak
Kolab Groupware Developer        [https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]
----------------------------------------------------
PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

Derick Rethans

3 years ago

On Thu, 27 Apr 2023, Máté Kocsis wrote:

> As you have possibly already experienced, overloaded signatures cause
> various smaller and bigger issues, while the concept is not natively
> supported by PHP. That's why I drafted an RFC which intends to phase
> out the majority of overloaded function/method signatures and also
> forbid the introduction of such functions in the future:
> https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures

I'm going to nitpick on the newly suggested names and argument order for the
DatePeriod factory methods — althoughI do agree that they need to get created:

createFromInterval(DateTimeInterface $start, DateInterval $interval, DateTimeInterface $end, int $options = 0)
→ createWithRange(DateTimeInterface $begin, DateTimeInterface $end, DateTimeInterface $int, int $options = 0)

createFromRecurrences(DateTimeInterface $start, DateInterval $interval, int $recurrences, int $options = 0)
→ createWithRecurrences(DateInterval $begin, int $recurrences, DateInterval $interval, int $options = 0)

We also should fix the argument names. Either $start/$finish, or $begin/$end. I
prefer the latter.

createFromIso8601(string $specification, int $options = 0)
-> createFromISO8601String

I am open to bike shedding about this :-)

cheers,
Derick

Tim Düsterhus

3 years ago

Hi

On 5/5/23 18:12, Derick Rethans wrote:

> createFromIso8601(string $specification, int $options = 0)
> -> createFromISO8601String
>
> I am open to bike shedding about this :-)

Okay, I'd like a different color of my bikeshed:

Please no consecutive uppercase letters in the function name. I find
createFromIso8601 (or createFromIso8601String if find the 'String'
suffix useful) much more readable than createFromISO8601String.
Uppercase acronyms are especially bad if followed by another ucfirst
word, because the word boundary is not immediately visible there. Not
the case here (it's digits), but still bad.

Best regards
Tim Düsterhus

Máté Kocsis

3 years ago

Hi Derick and Tim,

Derick, I incorporated most of your suggestions into the RFC with the sole
exception of the argument
names ($start/$finish,$begi/$end): it would feel odd if these parameters
and their related properties
were called differently:
https://www.php.net/manual/en/class.dateperiod.php#dateperiod.props.start

Actually, when we renamed the majority of parameters together with Nikita,
this was one of few rules that we had:
parameters should be called the same way as the property they initialize.

> Please no consecutive uppercase letters in the function name. I find
> createFromIso8601 (or createFromIso8601String if find the 'String'
> suffix useful) much more readable than createFromISO8601String.
> Uppercase acronyms are especially bad if followed by another ucfirst
> word, because the word boundary is not immediately visible there. Not
> the case here (it's digits), but still bad.

Yes, generally, I wholeheartedly agree, but unfortunately I still "had to"
go
with Derick's suggestion of using consecutive uppercase letters because
there
is already a DateTime::setISODate() method with the same casing and I want
to
follow the established naming convention of ext/date. Personally, I like
the "String"
suffix, probably it makes the name better, but since I don't really care
about this
problem too much, I'd leave it for Derick to decide about it.

Máté

Máté Kocsis

3 years ago

Hi Everyone,

Yesterday, I updated the voting choices based on Rowan's suggestions. I
also picked a few signatures where
only one migration path is proposed (the short one).

Now, I'd be curious what to do with the FFI methods in question (
https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#fficast_ffinew_and_ffitype).
..
Especially, it would be very nice to hear about FFI users' opinions.

Also, there was feedback about the session_set_save_handlers() function
name that it shouldn't resemble this
much to the original session_set_save_handler(). Rowan suggested using the
session_set_save_handler_functions() name.
While I'm not a big fan of this choice, I'd be interested in if there is
any other idea?

Particularly, I have been wondering for a long time, why the original
function includes "save_handler" in its name?
The passed in handlers are not just "save", but also other kinds of
handlers (e.g. "read"). So I'm considering to use
something like "session_set_handlers()" or
"session_set_handler_callbacks()". What do you think about these names?

If we manage to answer these questions then I'd like to start the vote not
long afterwards.

Regards,
Máté

Rowan Collins

3 years ago

On 17 May 2023 08:35:18 BST, "Máté Kocsis" <kocsismate90@gmail.com> wrote:

>Particularly, I have been wondering for a long time, why the original
>function includes "save_handler" in its name?
>The passed in handlers are not just "save", but also other kinds of
>handlers (e.g. "read"). So I'm considering to use
>something like "session_set_handlers()" or
>"session_set_handler_callbacks()". What do you think about these names?

I think that's a very good point, and I like the explicitness of "session_set_handler_callbacks" - this isn't a name people are going to need to type often, so there's not much reason to keep it short.

If we go down that route, perhaps we should come up with a corresponding name for the object based version - "session_set_handler_object" perhaps? That would also mean the deprecation messages can be much simpler: if you're using the old name, you need to do something.

(There's another minor fringe benefit: once the old name is removed, it becomes available for users to polyfill if for some reason they're struggling to change code that calls it.)

I'm still in two minds on the general concept of this RFC, as it is placing a burden on users for the mostly minor convenience of maintainers; but I think you've done a good job responding to concerns and improving the proposal.

Regards,
Hi Máté,

Sorry I didn't get round to replying to this sooner, particularly this point:

--
Rowan Tommins
[IMSoP]

Nicolas Grekas

3 years ago

>
> > As you have possibly already experienced, overloaded signatures cause
> > various smaller and bigger issues, while the concept is not natively
> > supported by PHP. That's why I drafted an RFC which intends to phase
> > out the majority of overloaded function/method signatures and also
> > forbid the introduction of such functions in the future:
> > https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures
>
> I'm going to nitpick on the newly suggested names and argument order for
> the
> DatePeriod factory methods — althoughI do agree that they need to get
> created:
>
> createFromInterval(DateTimeInterface $start, DateInterval $interval,
> DateTimeInterface $end, int $options = 0)
> → createWithRange(DateTimeInterface $begin, DateTimeInterface $end,
> DateTimeInterface $int, int $options = 0)
>
> createFromRecurrences(DateTimeInterface $start, DateInterval $interval,
> int $recurrences, int $options = 0)
> → createWithRecurrences(DateInterval $begin, int $recurrences,
> DateInterval $interval, int $options = 0)
>
> We also should fix the argument names. Either $start/$finish, or
> $begin/$end. I
> prefer the latter.
>
> createFromIso8601(string $specification, int $options = 0)
> -> createFromISO8601String
>
> I am open to bike shedding about this :-)
>

On my side, I'd very much prefer keeping the constructor of DatePeriod and
thus making it non-overloaded with this signature:

public function __construct(DateTimeInterface $start, DateInterval
$interval, DateTimeInterface|int $end, int $options = 0) {}

That'd help a lot with the migration/BC path and this signature also just
makes sense.

Unrelated: I don't think adding "ReflectionMethod::createFromMethodName" is
useful. Using explode is just fine for those that need this style. And
since this is the recommended migration path, better not give a reason to
change twice the code.

I like everything else in the RFC. I'm going to go with the "short path" of
deprecating things because the migration paths look smooth enough, thanks
for taking the time to explain them all (I just have a concern with the
DatePeriod migration path, which would be fixed with my suggestion bove.)

Thanks!
Nicolas

Máté Kocsis

3 years ago

Hi Nicolas,

On my side, I'd very much prefer keeping the constructor of DatePeriod and

> thus making it non-overloaded with this signature:
>
> public function __construct(DateTimeInterface $start, DateInterval
> $interval, DateTimeInterface|int $end, int $options = 0) {}
>
> That'd help a lot with the migration/BC path and this signature also just
> makes sense.
>

I completely agree with this (more so since this used to be the original
proposal), so I'll
ask Derick again about his stance on this.

Unrelated: I don't think adding "ReflectionMethod::createFromMethodName" is

> useful. Using explode is just fine for those that need this style. And
> since this is the recommended migration path, better not give a reason to
> change twice the code.
>

I don't really mind removing ReflectionMethod::createFromMethodName, so if
anyone has a
use-case which heavily relies on creating the reflection method from a
callable name ("Class::method" format),
now is the time to speak up for this method.

As far as I see the relevant impact analysis file (
https://gist.github.com/kocsismate/b457f8fef074039b58fbee9121d05e92),
Laminas, Symfony and Nette DI would also be impacted by the removal. I
don't think that it causes a big problem to
explode the string instead, but I'm wondering if performance sensitive code
may be negatively affected (?).

Thank you, Nicolas, for your insights!
Máté

Derick Rethans

3 years ago

On Tue, 13 Jun 2023, Nicolas Grekas wrote:

> > I'm going to nitpick on the newly suggested names and argument order for
> > the
> > DatePeriod factory methods — althoughI do agree that they need to get
> > created:
> >
> > createFromInterval(DateTimeInterface $start, DateInterval $interval,
> > DateTimeInterface $end, int $options = 0)
> > → createWithRange(DateTimeInterface $begin, DateTimeInterface $end,
> > DateTimeInterface $int, int $options = 0)
> >
> > createFromRecurrences(DateTimeInterface $start, DateInterval $interval,
> > int $recurrences, int $options = 0)
> > → createWithRecurrences(DateInterval $begin, int $recurrences,
> > DateInterval $interval, int $options = 0)
> >
> > We also should fix the argument names. Either $start/$finish, or
> > $begin/$end. I
> > prefer the latter.
> >
> > createFromIso8601(string $specification, int $options = 0)
> > -> createFromISO8601String
> >
> > I am open to bike shedding about this :-)
> >
>
> On my side, I'd very much prefer keeping the constructor of DatePeriod and
> thus making it non-overloaded with this signature:
>
> public function __construct(DateTimeInterface $start, DateInterval
> $interval, DateTimeInterface|int $end, int $options = 0) {}

That still has an overloaded third argument — DateTimeInterface|int.

Where you trying to suggest to change the current existing __construct()
to:

public function __construct(DateTimeInterface $start, DateInterval $interval, DateTimeInterface $end, int $options = 0)

and then add these two new factory methods?

createFromRecurrences(DateTimeInterface $start, DateInterval $interval, int $recurrences, int $options = 0)

createFromISO8601String(string $specification, int $options = 0)

That works for me.

cheers,
Derick

--
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support
Host of PHP Internals News: https://phpinternals.news

mastodon: @derickr@phpc.social @xdebug@phpc.social
twitter: @derickr and @xdebug

Unnamed Person

3 years ago

On Wed, Jun 14, 2023 at 4:06 AM Derick Rethans <derick@php.net> wrote:

>
> On Tue, 13 Jun 2023, Nicolas Grekas wrote:
>
> > > I'm going to nitpick on the newly suggested names and argument order for
> > > the
> > > DatePeriod factory methods — althoughI do agree that they need to get
> > > created:
> > >
> > > createFromInterval(DateTimeInterface $start, DateInterval $interval,
> > > DateTimeInterface $end, int $options = 0)
> > > → createWithRange(DateTimeInterface $begin, DateTimeInterface $end,
> > > DateTimeInterface $int, int $options = 0)
> > >
> > > createFromRecurrences(DateTimeInterface $start, DateInterval $interval,
> > > int $recurrences, int $options = 0)
> > > → createWithRecurrences(DateInterval $begin, int $recurrences,
> > > DateInterval $interval, int $options = 0)
> > >
> > > We also should fix the argument names. Either $start/$finish, or
> > > $begin/$end. I
> > > prefer the latter.
> > >
> > > createFromIso8601(string $specification, int $options = 0)
> > > -> createFromISO8601String
> > >
> > > I am open to bike shedding about this :-)
> > >
> >
> > On my side, I'd very much prefer keeping the constructor of DatePeriod and
> > thus making it non-overloaded with this signature:
> >
> > public function __construct(DateTimeInterface $start, DateInterval
> > $interval, DateTimeInterface|int $end, int $options = 0) {}
>
> That still has an overloaded third argument — DateTimeInterface|int.

This specific case is not that problematic. Since things need
untangling anyway, I would feel free to improve it if something is
done, but it's not much of a problem.

The problem is when there's a wildly different signature, or if args
change the return type structure, etc. So in this case, the problem is
really:

public function __construct(string $isostr, int $options = 0) {}

Which can't really be unified with the others very well.

Nicolas Grekas

3 years ago

> > > I'm going to nitpick on the newly suggested names and argument order
> for
> > > the
> > > DatePeriod factory methods — althoughI do agree that they need to get
> > > created:
> > >
> > > createFromInterval(DateTimeInterface $start, DateInterval $interval,
> > > DateTimeInterface $end, int $options = 0)
> > > → createWithRange(DateTimeInterface $begin, DateTimeInterface $end,
> > > DateTimeInterface $int, int $options = 0)
> > >
> > > createFromRecurrences(DateTimeInterface $start, DateInterval $interval,
> > > int $recurrences, int $options = 0)
> > > → createWithRecurrences(DateInterval $begin, int $recurrences,
> > > DateInterval $interval, int $options = 0)
> > >
> > > We also should fix the argument names. Either $start/$finish, or
> > > $begin/$end. I
> > > prefer the latter.
> > >
> > > createFromIso8601(string $specification, int $options = 0)
> > > -> createFromISO8601String
> > >
> > > I am open to bike shedding about this :-)
> > >
> >
> > On my side, I'd very much prefer keeping the constructor of DatePeriod
> and
> > thus making it non-overloaded with this signature:
> >
> > public function __construct(DateTimeInterface $start, DateInterval
> > $interval, DateTimeInterface|int $end, int $options = 0) {}
>
> That still has an overloaded third argument — DateTimeInterface|int.
>

Where you trying to suggest to change the current existing __construct()

> to:
>
> public function __construct(DateTimeInterface $start, DateInterval
> $interval, DateTimeInterface $end, int $options = 0)
>
> and then add these two new factory methods?
>
> createFromRecurrences(DateTimeInterface $start, DateInterval $interval,
> int $recurrences, int $options = 0)
>
> createFromISO8601String(string $specification, int $options = 0)
>

I really meant DateTimeInterface|int, not an overloaded signature but a
union type.

My concern is about providing the best path forward.
Both deprecating and providing the alternative in the same version creates
a lot of friction for end users.
After a quick chat with Mate, I think we should do this union type *in
addition* to adding the new static constructors.
Then, later on, if future us think it's worth it, we might want to consider
deprecating passing an integer.
This would provide a path for BC/FC I would fully support.

Nicolas

Derick Rethans

3 years ago

On Mon, 19 Jun 2023, Nicolas Grekas wrote:

> > > On my side, I'd very much prefer keeping the constructor of
> > > DatePeriod and thus making it non-overloaded with this signature:
> > >
> > > public function __construct(DateTimeInterface $start, DateInterval
> > > $interval, DateTimeInterface|int $end, int $options = 0) {}
> >
> > That still has an overloaded third argument — DateTimeInterface|int.
> >
> > Where you trying to suggest to change the current existing
> > __construct() to:
> >
> > public function __construct(DateTimeInterface $start, DateInterval
> > $interval, DateTimeInterface $end, int $options = 0)
> >
> > and then add these two new factory methods?
> >
> > createFromRecurrences(DateTimeInterface $start, DateInterval $interval,
> > int $recurrences, int $options = 0)
> >
> > createFromISO8601String(string $specification, int $options = 0)
> >
>
> I really meant DateTimeInterface|int, not an overloaded signature but a
> union type.

You may call it a union type, but that's still an overloaded signature :-)

> My concern is about providing the best path forward. Both deprecating
> and providing the alternative in the same version creates a lot of
> friction for end users.
>
> After a quick chat with Mate, I think we should do this union type *in
> addition* to adding the new static constructors.
>
> Then, later on, if future us think it's worth it, we might want to
> consider deprecating passing an integer.
>
> This would provide a path for BC/FC I would fully support.

In any case, I don't mind this — I'm actually going to suggest to change
the constructor to:

public function __construct(DateTimeInterface $start, DateInterval $interval, DateTimeInterface|int $end, int $options = 0) {}

And then *only* add:

public static function createFromISO8601String(string $specification, int $options = 0): static {}

This solves the original problem of not being able to define the
signatures in stubs, and also extracts the most problematic of methods
into a factory method where it should always have belonged.

There would be no additional benefit in creating the other two factory
methods.

cheers,
Derick

Máté Kocsis

3 years ago

Hi Derick,

In any case, I don't mind this — I'm actually going to suggest to change

> the constructor to:
>
> public function __construct(DateTimeInterface $start, DateInterval
> $interval, DateTimeInterface|int $end, int $options = 0) {}
>
> And then *only* add:
>
> public static function createFromISO8601String(string $specification, int
> $options = 0): static {}
>
> This solves the original problem of not being able to define the
> signatures in stubs, and also extracts the most problematic of methods
> into a factory method where it should always have belonged.
>

Thanks for your response, I've just updated the RFC accordingly. It solves
the main issue indeed, and we still have the possibility to
add a second factory method using the $recurrences parameter later if we
deem it useful at some point.

With all that said, I don't want to update the RFC anymore, so I plan to
start the vote on Monday.

Regards,
Máté

Claude Pache

3 years ago

> Le 27 avr. 2023 à 23:28, Máté Kocsis <kocsismate90@gmail.com> a écrit :
>
> Hi Internals,
>
> As you have possibly already experienced, overloaded signatures cause
> various smaller and bigger issues, while the concept is not natively
> supported by PHP. That's why I drafted an RFC which intends to phase out
> the majority of overloaded function/method signatures and also forbid
> the introduction of such functions in the future:
> https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures
>
>

Hi Máté,

The replacement methods for IntlCalendar::set() (namely IntlCalendar::setDate() and IntlCalendar::setDateTime()) must not have a return type of `void`, but `true`, like the original method, for the two following reasons:

1. By changing the returned value, you are introducing an unnecessary hazard when migrating code.

2. Per https://www.php.net/IntlCalendar, all modification methods of that class (clear(), roll(), setTime(), etc.) consistently return `true` on success and `false` on failure, even when the method is infallible (and thus would always return `true`). Don’t break consistency.

—Claude

Máté Kocsis

3 years ago

Hi Claude,

The replacement methods for IntlCalendar::set() (namely

> IntlCalendar::setDate() and IntlCalendar::setDateTime()) must not have a
> return type of `void`, but `true`, like the original method, for the two
> following reasons:
>
> 1. By changing the returned value, you are introducing an unnecessary
> hazard when migrating code.
>
> 2. Per https://www.php.net/IntlCalendar, all modification methods of that
> class (clear(), roll(), setTime(), etc.) consistently return `true` on
> success and `false` on failure, even when the method is infallible (and
> thus would always return `true`). Don’t break consistency.
>

1.  I don't see much risk with this change, since we clearly indicate at
the very beginning that the new methods return void, so programmers
migrating their code
should pay attention to the return types. IDEs and static analysers can
surely warn them in case of inattention.

2. Some of the methods you mentioned have a "true" return type for a few
weeks now. The biggest motivation for introducing these was to at least
have some chance to
make them void in the future. Doing so is a risky move indeed, so should be
carried out very carefully, if it's possible at all... That's why I
consider it beneficial to spare the
effort and start with a clean slate by adding the new methods in question
with their correct return type.

Regards,
Máté

Claude Pache

3 years ago

Hi Máté,

> Le 30 mai 2023 à 01:41, Máté Kocsis <kocsismate90@gmail.com> a écrit :
>
> Hi Claude,
>
>> The replacement methods for IntlCalendar::set() (namely IntlCalendar::setDate() and IntlCalendar::setDateTime()) must not have a return type of `void`, but `true`, like the original method, for the two following reasons:
>>
>> 1. By changing the returned value, you are introducing an unnecessary hazard when migrating code.
>>
>> 2. Per https://www.php.net/IntlCalendar, all modification methods of that class (clear(), roll(), setTime(), etc.) consistently return `true` on success and `false` on failure, even when the method is infallible (and thus would always return `true`). Don’t break consistency.
>
> 1.  I don't see much risk with this change, since we clearly indicate at the very beginning that the new methods return void, so programmers migrating their code
> should pay attention to the return types. IDEs and static analysers can surely warn them in case of inattention.

The very risk is that the programmer must be aware that the return convention has changed. Recall that not everyone run static analysers, especially for such a trivial migration, and that not every code is statically analysable (e.g.. `$foo->$bar()`).

Of course, the benefice of the change might be worth the risk; but that leads to the second point:

>
> 2. Some of the methods you mentioned have a "true" return type for a few weeks now. The biggest motivation for introducing these was to at least have some chance to
> make them void in the future. Doing so is a risky move indeed, so should be carried out very carefully, if it's possible at all... That's why I consider it beneficial to spare the
> effort and start with a clean slate by adding the new methods in question with their correct return type.

Concretely, if you change the return value (from `true` to `void/null`), you will break the following pattern:

<?php
if (!@$foo->setBar(...)) {
// error handling (might be dead code, it doesn’t matter)
}
?>

Any change that introduce a BC break or a migration burden must be justified. (There has been enough rant around justified BC breaks, so let’s not introduce unjustified ones.)

What is the purpose of changing the return convention of IntlCalendar methods, and is it worth?

—Claude

Máté Kocsis

3 years ago

Hi Claude,

The very risk is that the programmer must be aware that the return

> convention has changed. Recall that not everyone run static analysers,
> especially for such a trivial migration, and that not every code is
> statically analysable (e.g.. `$foo->$bar()`).
>

I hope that I don't sound elitist, but codebases not using static
analysis... are kind of hopeless... That is, in my opinion, there's no
other option
to run a larger PHP project successfully than to either use static analysis
or to have programmers who don't commit any mistakes. For the rest
of the less valuable projects, the cost of messing up with the return value
check is not much.

What is the purpose of changing the return convention of IntlCalendar

> methods, and is it worth?
>

The purpose is to have the "correct" return type. Is it worth it? Well, it
depends... For me, what's important is that PHP becomes a more and
more predictable and accessible language, and I care less about minimizing
backward compatibility breaks.

The introduction of the "true" type was useful, since we now have more
correct type information, but adding new functions with this return type
would be unfortunate, as many people (especially the ones coming from other
languages) would be puzzled about the meaning of
the "true" type more than if the return type was "void". I'm pretty sure
that George - the author of the true type - agrees that the main purpose
of this type is to eliminate its declarations from internal functions and
methods.

Regards,
Máté

Rowan Collins

3 years ago

On 2 June 2023 14:21:49 BST, "Máté Kocsis" <kocsismate90@gmail.com> wrote:

>I hope that I don't sound elitist, but codebases not using static
>analysis... are kind of hopeless...

Then I guess we should just pack up and go home, because right now PHP doesn't even have an official static analyser, let alone a mandatory one; and the evolution of Hack shows just how much the language would need to change for such a tool to guarantee full coverage.

The needs of new users being introduced to best practices are different from those maintaining and modernising existing codebases. Sometimes, they're in direct opposition, but I think we have a duty to try to support both where we can.

> The purpose is to have the "correct" return type. Is it worth it? Well, it home
> depends... For me, what's important is that PHP becomes a more and
> more predictable and accessible language, and I care less about minimizing
> backward compatibility breaks.

I think Claude is taking the same premise, and reaching a  different conclusion: returning true is consistent with the other methods on the class, and that consistency makes it more predictable and more accessible.

Regards,

--
Rowan Tommins
[IMSoP]

Girgias

3 years ago

On Tue, 30 May 2023 at 09:54, Claude Pache <claude.pache@gmail.com> wrote:

> Any change that introduce a BC break or a migration burden must be
> justified. (There has been enough rant around justified BC breaks, so let’s
> not introduce unjustified ones.)
>
> What is the purpose of changing the return convention of IntlCalendar
> methods, and is it worth?
>

We are talking about adding a new method here, so I don't really see how
this is a BC break.
One would hope that people do not just blindly change function names,
especially considering that one would need to look at the migration guide,
where it can be explicitly noted that the return type is different from the
previous method.

Moreover, the return convention of IntlCalendar (and most other Intl
classes) has for the most par always been wrong.
Prior to PHP 8.0, passing invalid types to functions/methods was considered
undefined behaviour, and in general the value returned when a TypeError
wasn't thrown was NULL.
However, Intl (and some other extensions) didn't follow this and returned
false on ZPP errors and always true otherwise.
As such, the change to *always* throw TypeErrors on ZPP violations has
brought to light those sorts of extensions.
This was one of the main motivation to add the true singleton type other
than consistency, as this allows tools like static analysis, but not
limited to them, to detect dead code and bring this up to people's
attention.

As effectively, there is no point in storing and/or comparing the value of
a method/function that always returns the same value (be that null(/void),
true, false, or other).
Considering this fact, that one should not compare the return value of most
(if not all) of these methods, using void as the type seems very much
appropriate rather than using true.

Best regards,

George P. Banyard

Unnamed Person

3 years ago

On 27-4-2023 23:28, Máté Kocsis wrote:

> Hi Internals,
>
> As you have possibly already experienced, overloaded signatures cause
> various smaller and bigger issues, while the concept is not natively
> supported by PHP. That's why I drafted an RFC which intends to phase out
> the majority of overloaded function/method signatures and also forbid
> the introduction of such functions in the future:
> https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures
>
> I'm not sure at all what the best solution would be to get rid of the
> overloaded FFI methods, so I hope that someone comes up with a better idea.
> Currently, the RFC suggests deprecating and then removing the static
> methods in question, but I hope that we can find a more elegant approach.
> So this section is incomplete and it's just for starting the discussion.
>
> Regards,
> Máté
>

The impact analysis on userland code seems to be missing for some of the
proposals, most notably for the proposals which I expect will have the
highest impact. I'd like to ask for an impact analysis to be added to
each of these:
* array_keys()
* ReflectionProperty::setValue()

For the `get*_class()` deprecation, I wonder if an additional impact
analysis is needed for packages which may not have removed usages of
`get_class( null )`, which would now be double-impacted (and not caught
by the current analysis).

Smile,
Juliette

Máté Kocsis

3 years ago

Hi,

The impact analysis on userland code seems to be missing for some of the

> proposals, most notably for the proposals which I expect will have the
> highest impact. I'd like to ask for an impact analysis to be added to
> each of these:
> * array_keys()
> * ReflectionProperty::setValue()
>

These are intentionally left out due to different reasons:

* Even though the 1 parameter version of array_keys() is used a lot, its 2+
parameter signature is much less known.
Personally, I've never used it. Of course, this doesn't mean no one uses
it, but I don't think this deprecation will really
matter in practice, so I didn't care to write a script for this (the rest
of the deprecations were easy to analyze via regexes).

* ReflectionProperty::setValue() would be a lot of work to analyze since
it's very difficult to track whether the
setValue() method is called on a ReflectionProperty instance. Not to
mention the fact that the deprecation only affects
function invocations where either a single parameter is provided or static
properties where the first parameter is not null
or an object instance. Since the suggested alternative is basically
available since forever, I think it's OK not to do
an exhaustive analysis in this case.

> For the `get*_class()` deprecation, I wonder if an additional impact
> analysis is needed for packages which may not have removed usages of
> `get_class( null )`, which would now be double-impacted (and not caught
> by the current analysis).
>

I'm not sure I can follow you, but get_class(null) is a TypeError since 8.0
(and have been warning since 7.2). If a package
didn't feel the need to migrate its get_class() usages then it is likely a
dead project. And if someone tries to upgrade their
(proprietary) code from PHP 7.1 to PHP 8.3 directly then they will notice
the exception first and also - if they followed the upgrading
notes carelly - will learn the relevant suggestions regarding the
deprecation. But realistically speaking, these projects will most probably
be bound by lots of other more severe issues, so I don't think the above is
a viable upgrade path...

Regards,
Máté

Unnamed Person

3 years ago

On 19-6-2023 23:16, Máté Kocsis wrote:

> Hi,
>
> The impact analysis on userland code seems to be missing for some of the
>> proposals, most notably for the proposals which I expect will have the
>> highest impact. I'd like to ask for an impact analysis to be added to
>> each of these:
>> * array_keys()
>> * ReflectionProperty::setValue()
>>
> These are intentionally left out due to different reasons:
>
> * Even though the 1 parameter version of array_keys() is used a lot, its 2+
> parameter signature is much less known.
> Personally, I've never used it. Of course, this doesn't mean no one uses
> it, but I don't think this deprecation will really
> matter in practice, so I didn't care to write a script for this (the rest
> of the deprecations were easy to analyze via regexes).
>
> * ReflectionProperty::setValue() would be a lot of work to analyze since
> it's very difficult to track whether the
> setValue() method is called on a ReflectionProperty instance. Not to
> mention the fact that the deprecation only affects
> function invocations where either a single parameter is provided or static
> properties where the first parameter is not null
> or an object instance. Since the suggested alternative is basically
> available since forever, I think it's OK not to do
> an exhaustive analysis in this case.
>
>
>> For the `get*_class()` deprecation, I wonder if an additional impact
>> analysis is needed for packages which may not have removed usages of
>> `get_class( null )`, which would now be double-impacted (and not caught
>> by the current analysis).
>>
> I'm not sure I can follow you, but get_class(null) is a TypeError since 8.0
> (and have been warning since 7.2). If a package
> didn't feel the need to migrate its get_class() usages then it is likely a
> dead project. And if someone tries to upgrade their
> (proprietary) code from PHP 7.1 to PHP 8.3 directly then they will notice
> the exception first and also - if they followed the upgrading
> notes carelly - will learn the relevant suggestions regarding the
> deprecation. But realistically speaking, these projects will most probably
> be bound by lots of other more severe issues, so I don't think the above is
> a viable upgrade path...
>
> Regards,
> Máté
>

Thanks for the reply Máté.

I understand what you are saying and appreciate it may be difficult to
get an accurate impact analysis for `ReflectionProperty::setValue()`.

Respectfully though, in my opinion selectively leaving out impact
analysis without mentioning why they are missing in the RFC, reeks of
trying to hide information which could influence the vote.
Maybe just mentioning why the impact analysis is missing in these cases
in the RFC could take that stench away ?

Smile,
Juliette

Máté Kocsis

3 years ago

Hi Juliette,

Respectfully though, in my opinion selectively leaving out impact analysis

> without mentioning why they are missing in the RFC, reeks of trying to hide
> information which could influence the vote.
> Maybe just mentioning why the impact analysis is missing in these cases in
> the RFC could take that stench away ?
>

I respectfully disagree that it would affect the vote result, but I added
the clarification you suggested nevertheless. :) Doing so resulted in a
very nice side-effect: I noticed that the array_keys_search() function name
is slightly inconsistent/misleading, since what it really does is to filter
keys based on their *value* (thus the $filter_value param name). So I
renamed it to array_keys_filter() as well as came up with
a new suggested replacement which is compatible with at least PHP 4: using
the combination of array_keys() and array_filter() retains the original
behavior (example is added to the RFC). I know
that it's less than ideal performance-wise, but it works anyway (and not
all code is performance sensitive).

Thanks,
Máté