Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122896
Content-Type: multipart/alternative;
 boundary="------------gHzdzIYZYfSLV7tyji5E0mgZ"
Message-ID: <98aafc64-4b78-4948-b2ba-6a36f22b9b67@scriptfusion.com>
Date: Tue, 2 Apr 2024 21:32:43 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PHP-DEV] First time contributor (DateTime::setDate PR)
To: Derick Rethans <derick@php.net>
Cc: internals@lists.php.net
References: <31f7de10-a2dd-489e-bc2f-66987dc832aa@scriptfusion.com>
 <d0294f5f-9e71-4a3c-10b4-1a5da4de4060@php.net>
Content-Language: en-GB
In-Reply-To: <d0294f5f-9e71-4a3c-10b4-1a5da4de4060@php.net>
From: bilge@scriptfusion.com (Bilge)

This is a multi-part message in MIME format.
--------------gHzdzIYZYfSLV7tyji5E0mgZ
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 02/04/2024 14:14, Derick Rethans wrote:
> Hi,
>
> On Sun, 31 Mar 2024, Bilge wrote:
>
>> About the PR: I sometimes find it would be useful to only update part of the
>> date. The PR makes all parameters to DateTime(Immutable)::setDate
>> <https://www.php.net/manual/en/datetimeimmutable.setdate.php>  optional in a
>> backwards-compatible manner such that we can elect to update only the day,
>> month, year or any combination of the three (thanks, in part, to named
>> parameters). Without this modification, we must always specify all of the day,
>> month and year parameters to change the date.
> As I mentioned to you in Room 11, I am not in favour of adhoc API
> changes to Date/Time classes. It has now been nearly 18 years since they
> were originally introduced, and they indeed could do with an overhaul.
>
> I have been colllecting ideas in
> https://docs.google.com/document/d/1pxPSRbfATKE4TFWw72K3p7ir-02YQbTf3S3SIxOKWsk/edit#heading=h.2jol7kfhmijb
>
> Having different/better modifiers would also be a good thing to talk
> about, albeit perhaps on the four mentioned new classes, instead of
> adding them to the already existing DateTime and DateTimeImmutable
> classes.
>
> In any case, just allowing setDate to be able to just modify the month
> is going to introduce confusion, as this will be counter intuitive:
>
> $dt = new DateTimeImmutable("2024-03-31");
> $newDt = $dt->setDate( month: 2 );
>
> It is now representing 2024-03-02.
>
> This might be the right answer, but it might also be that the developer
> just cared about the month part (and not the day-of-month), in which
> case this is a WTF moment.
>
> Picking mofication APIs is not as trivial as it seems, and I would like
> to do it *right*.
>
> Feel free to add comments and wishes to the google doc document. In the
> near future, I will be writing up an RFC from this.
>
> cheers,
> Derick
Hi Derick,

Thanks for your reply!

Indeed, as per your code snippet, this is a WTF moment I had not 
accounted for and confirm the same result with my patch applied. 
Generally, my expectation here would be the month *must* be set to 2, so 
if the day portion will be invalidated by that change, we should either 
throw an exception or implicitly coerce it into range, i.e. (new 
DateTime("2024-03-31"))->setDate(month: 2); == 2024-02-29. However, I 
suppose this is not the conversation we're having as you do not wish to 
change this API at all, which I respect.

Regarding your brainstorm document, I can't understand much of it in its 
current state, and as I am not a subject matter expert, I think you will 
receive much better feedback from others. In particular, I cannot glean 
which four classes you are referring to in that document. Yet what I do 
find interesting is the notion of adding setters to DateTimeImmutable. 
For my particular use-case—producing a collection of dates incrementing 
by year in a Twig template—a trivial year setter would do just fine, 
with the significant caveat that it must implement fluent interface, 
because I need to call it in an expression context (returning a value), 
not a statement context (executing a void function separately). Not that 
Twig cannot execute statements, but it just becomes more verbose, 
cumbersome and less template-like.

If you were happy for me to add getters and fluent setters for year, I'd 
be happy to work on that PR, but for month we're back to the same 
problem outlined in the opening paragraph (and I suppose the same 
problem occasionally applies to year, if the day happens to be set to 
the leap day). Otherwise, I'll be happy to read over your RFC when it's 
ready.

Kind regards, Bilge

--------------gHzdzIYZYfSLV7tyji5E0mgZ
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    On 02/04/2024 14:14, Derick Rethans wrote:<br>
    <blockquote type="cite"
      cite="mid:d0294f5f-9e71-4a3c-10b4-1a5da4de4060@php.net">
      <pre class="moz-quote-pre" wrap="">Hi,

On Sun, 31 Mar 2024, Bilge wrote:

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">About the PR: I sometimes find it would be useful to only update part of the
date. The PR makes all parameters to DateTime(Immutable)::setDate
<a class="moz-txt-link-rfc2396E" href="https://www.php.net/manual/en/datetimeimmutable.setdate.php">&lt;https://www.php.net/manual/en/datetimeimmutable.setdate.php&gt;</a> optional in a
backwards-compatible manner such that we can elect to update only the day,
month, year or any combination of the three (thanks, in part, to named
parameters). Without this modification, we must always specify all of the day,
month and year parameters to change the date.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
As I mentioned to you in Room 11, I am not in favour of adhoc API 
changes to Date/Time classes. It has now been nearly 18 years since they 
were originally introduced, and they indeed could do with an overhaul.

I have been colllecting ideas in 
<a class="moz-txt-link-freetext" href="https://docs.google.com/document/d/1pxPSRbfATKE4TFWw72K3p7ir-02YQbTf3S3SIxOKWsk/edit#heading=h.2jol7kfhmijb">https://docs.google.com/document/d/1pxPSRbfATKE4TFWw72K3p7ir-02YQbTf3S3SIxOKWsk/edit#heading=h.2jol7kfhmijb</a>

Having different/better modifiers would also be a good thing to talk 
about, albeit perhaps on the four mentioned new classes, instead of 
adding them to the already existing DateTime and DateTimeImmutable 
classes.

In any case, just allowing setDate to be able to just modify the month 
is going to introduce confusion, as this will be counter intuitive:

$dt = new DateTimeImmutable("2024-03-31");
$newDt = $dt-&gt;setDate( month: 2 );

It is now representing 2024-03-02.

This might be the right answer, but it might also be that the developer 
just cared about the month part (and not the day-of-month), in which 
case this is a WTF moment.

Picking mofication APIs is not as trivial as it seems, and I would like 
to do it *right*.

Feel free to add comments and wishes to the google doc document. In the 
near future, I will be writing up an RFC from this.

cheers,
Derick
</pre>
    </blockquote>
    Hi Derick,
    <p>Thanks for your reply!</p>
    <p>Indeed, as per your code snippet, this is a WTF moment I had not
      accounted for and confirm the same result with my patch applied.
      Generally, my expectation here would be the month *must* be set to
      2, so if the day portion will be invalidated by that change, we
      should either throw an exception or implicitly coerce it into
      range, i.e. <span style="white-space: pre-wrap"><font
      face="monospace">(new DateTime("2024-03-31"))-&gt;setDate(month: 2);</font> == </span><span
      style="white-space: pre-wrap">2024-02-29. However, I suppose this is not the conversation we're having as you do not wish to change this API at all, which I respect.</span></p>
    <p><span style="white-space: pre-wrap">Regarding your brainstorm document, I can't understand much of it in its current state, and as I am not a subject matter expert, I think you will receive much better feedback from others. In particular, I cannot glean which four classes you are referring to in that document. Yet what I do find interesting is the notion of adding setters to <font
      face="monospace">DateTimeImmutable</font>. For my particular use-case</span><span
        class="LEwnzc Sqrs4e">—</span><span
      style="white-space: pre-wrap">producing a collection of dates incrementing by year in a Twig template</span><span
        class="LEwnzc Sqrs4e">—</span><span
      style="white-space: pre-wrap">a trivial year setter would do just fine, with the significant caveat that it must implement fluent interface, because I need to call it in an expression context (returning a value), not a statement context (executing a void function separately). Not that Twig cannot execute statements, but it just becomes more verbose, cumbersome and less template-like.</span></p>
    <p><span style="white-space: pre-wrap">If you were happy for me to add getters and fluent setters for year, I'd be happy to work on that PR, but for month we're back to the same problem outlined in the opening paragraph (and I suppose the same problem occasionally applies to year, if the day happens to be set to the leap day). Otherwise, I'll be happy to read over your RFC when it's ready.</span></p>
    <p><span style="white-space: pre-wrap">Kind regards,
Bilge</span><span style="white-space: pre-wrap">
</span></p>
  </body>
</html>

--------------gHzdzIYZYfSLV7tyji5E0mgZ--