[RFC] [Discussion] Deprecate functions with overloaded signatures

Hi

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

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é

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é

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]

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.

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

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é

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

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é

Given that you've agreed that neither signature is "primary" or "better",
doesn't that argument apply equally to both signatures? If it's OK to force
anyone using individual callbacks to change their code, why is it not
equally OK to force anyone using an object to change their code?

As far as I remember, my reasoning was two-fold:

• naming: passing multiple callbacks to a function name in singular
  (session_set_save_handler())
  is definitely not ideal
• a decision had to be made and I thought that keeping the OO approach in
  place had less BC impact,
  especially because previously I got feedback from Nicolas that they used
  this version.

I do wonder if this change is too disruptive relative to the benefit it
brings, but in that case, we should just leave the whole function as it is.

In my opinion, phasing out session_set_save_handler() completely would be
worth the effort if this
was the only option to fix the overridden signature. However, it's not the
case, since strictly speaking,
only one of the two signatures has to be removed in order to achieve the
desired goal of the RFC.
The whole discussion about also deprecating the other one started only
because of improving naming:
it is also a nice thing to pursue but fails the cost-benefit analysis. All
in all, I think neither not doing
anything, nor deprecating the whole function is a good choice. But at least
I definitely want the question
to be settled with putting this to vote.

I don't think "handler" particularly implies "object". The "handler" passed

to set_error_handler is a function, and your original suggestion for a new
function was "session_set_save_handlers", where "handler" would also mean
"function".

Without any suffix at all, it seems like "set a session handler the normal

way" vs "set a session a special different way"; like how
"array_merge_recursive" is a special version of "array_merge".

I think my reasoning is easier to understand if we go back to my original
suggestion:
session_set_save_handler() and session_set_save_handlers(), which would be
session_set_handler() and session_set_handlers() now. That was the starting
point,
but you didn't like that they only differ by an "s". That's why I swapped
the "s" with "callbacks".

According to your deduction, session_set_handler() is not the most correct
name indeed, but I don't think
that we always have to choose the most correct and the entirely descriptive
one. After all, neither the
parameters of session_set_handler() are called "$open_callback",
"$close_callback" etc., they are just
"$open", "$close" etc. and I guess they are still clear enough, given that
their type is declared which
completes the missing context.

Similarly, we all would know that the session_set_handler() function
expects an object of
SessionHandlerInterface type, while its sibling,
session_set_handler_callbacks() expects some
callbacks. Yes, having the _object suffix is the most pedantic name, but
personally, I don't really like it
because I find it ugly (and I was pretty much content with
session_set_handlers()). I'm curious about
the point of view of other people though, because if it turns out that
people generally also favor some
other name then I'm ok with a compromise.