Typed constants in date extension #12361
Conversation
jorgsowa
force-pushed
the
typed-constants-in-date-extension
branch
from
3fa369f to
01201f7
Compare
|
LGTM but let's wait for Derick's review |
ext/date/php_date.stub.php
Outdated
| /** | ||
| * @tentative-return-type | ||
| */ | ||
| /** @tentative-return-type */ |
There was a problem hiding this comment.
Please don't make unnecessary formatting changes.
There was a problem hiding this comment.
Actually, IMO this is an useful change, makes the stub much shorter and easier to overview. Actually, I asked the same in different extensions.
There was a problem hiding this comment.
They shouldn't be in the same commit (which it will end up in after squashing), and hence, shouldn't be part of this PR to begin with.
| * @cvalue PHP_DATE_TIMEZONE_PER_COUNTRY | ||
| */ | ||
| public const PER_COUNTRY = UNKNOWN; | ||
| /** @cvalue PHP_DATE_TIMEZONE_GROUP_AFRICA */ |
There was a problem hiding this comment.
Please don't make unnecessary formatting changes (just remove the @var int line).
ext/date/php_date.stub.php
Outdated
| /** | ||
| * @tentative-return-type | ||
| */ | ||
| /** @tentative-return-type */ |
There was a problem hiding this comment.
Please don't make unnecessary formatting changes.
ext/date/php_date.stub.php
Outdated
| /** | ||
| * @strict-properties | ||
| */ | ||
| /** @strict-properties */ |
There was a problem hiding this comment.
Please don't make unnecessary formatting changes.
Yes it is a BC break, that's why the change is mentioned in the UPRADING file. BTW why would you want to overwrite ext/date's constants? Do you have a legitimate use-case? |
jorgsowa
force-pushed
the
typed-constants-in-date-extension
branch
from
01201f7 to
16675d6
Compare
|
@derickr, thank you for the review. I agree this is BC, but it's small enough so I thought it's safe to merge. I think Date extension is the last one that has not those kind of changes merged. Regarding the comments I followed similar convention in other extensions, but other comments in the file as well, where comments of 1 line, where changed to 1-line PHPDoc. Similarly to the lines 662 and 665. https://github.com/php/php-src/pull/12361/files#diff-69c5b7520e110ac19361b7b043c1e2b75b8b11c9154557b1927491db2a47643aL662 Should I change it back? |
|
@derickr Could you please do another review round? Basically all the other internal class constants are typed now, so it would be nice to do the same in ext/date |
jorgsowa
force-pushed
the
typed-constants-in-date-extension
branch
from
16675d6 to
114422f
Compare
ext/date/php_date.stub.php
Outdated
| /** | ||
| * @tentative-return-type | ||
| */ | ||
| /** @tentative-return-type */ |
There was a problem hiding this comment.
They shouldn't be in the same commit (which it will end up in after squashing), and hence, shouldn't be part of this PR to begin with.
|
Thanks, @derickr for the review. I have reverted unrelated PHPDoc changes. I will create another PR for them. |
|
This is breaking, see. https://github.com/php/php-src/actions/runs/9200769315/job/25307893880 I think it's a bit early to add these types, because libraries supporting older PHP versions (that are still supported too) won't have a way to override these constants, given that typed class constants were only introduced in PHP 8.3. |
|
There is currently no way to deprecate changing the type of the properties so I assume the proper way would be to apply type to new constants only. I will prepare PR with a revert. |
|
That said, I don't know why Carbon overrides this constant. Maybe it's a BC layer that's no longer needed? It has the same value as our constant. If you prefer, we can ping them first to see if it is still needed. |
|
I did say this was a BC break, but you all didn't think it was important then!
|
|
I think the BC break would be fine if there was an easy, backwards compatible solution. As of now, I'd prefer reverting this. But I'm not owner of this code, so I'll leave it up to you and @kocsismate. |
|
I had a quick look at their constant usage, and they could have used any other constant... Especially because their override doesn't have any effect on the internal code. So I am wondering if we could selectively revert only the type declaration of |
|
Thanks @kocsismate. Please revert this for now then, so that the build works again. Feel free to ping the Carbon authors and ask whether this constant can be removed on their side, and then reapply accordingly. |
|
@kocsismate You can wait with reverting this. I just now saw that these changes have already been made to many extensions and it doesn't make sense singling out one extension. I still think it would have been better to wait another release to make these changes, so that a PHP code base can change the signature of overridden constants by adding a type and supporting all officially supported versions. E.g. they may want to add an attribute, which I would consider a valid use-case. Anyway, we should either revert all or none of them. I'm creating a PR for Carbon in the meantime. |
|
Hello everybody, thank you for making me aware of this upcoming change, I'll investigate if this is an issue on our side and get back to you soon. |
|
So we can safely remove our override so our next version won't have any problem with that and we are not PHP 8.4 ready for other reasons so it does not make a difference neither for our previous versions. No blocker here. Thank you very much for reaching out. 🙏 |
|
@kylekatarnls @iluuu1994 Thank you both for taking care of this problem :) I was busy until now with life, but I'm happy to see that the problem is gone. |
This PR adds types to the constants in Date extension.