Conversation
78d78d5 to
36cdddc
Compare
36cdddc to
385cadb
Compare
385cadb to
3ef2e00
Compare
db9b88b
db9b88b to
0f5ecc3
Compare
5a0b140 to
63dee37
Compare
lippserd
left a comment
There was a problem hiding this comment.
In addition to the requested changes, I have a few comments on the commit messages and descriptions:
For 982ef28, please use something like the following, as suggested in Icinga/ipl-stdlib#61 (review):
PHP 8.4: Change implicit nullable type declaration to explicit
Since PHP 8.4 implicitly nullable parameter types are deprecated.
The reason for upgrading guzzlehttp/psr7 in d5b0989 is misleading, as version 2.8.0 introduces PHP 8.5 support and is a dependency for this library. In general, our ipl-* repositories build our Icinga PHP library, not the other way around. Therefore, we should never have such "streamline" commits. Something similar was also discussed/addressed in Icinga/ipl-validator#38.
For 41413f0, the commit message could be worded more precisely, and a description justifying the removal should be added, e.g.:
PHP 8.5: Remove deprecated method `ReflectionProperty::setAccessible()`
Since we require PHP >= 8.2 and since calling this method has no effect as of
PHP 8.1.0, as all properties are accessible by default, it is safe to simply
remove this method call.
Finally, 3ef2e00 could be rephrased to PHP 8.5: Replace usage of `null` as array offset. A description that only contains a reference to the PHP documentation is not necessary, but something like the following:
Since PHP 8.5 using `null` as an array offset is deprecated. Code relying on
`null` being silently converted to an empty string must be replaced and
discouraged in PHPDocs.
|
And please also add a PR description. |
217ad5b to
83c1631
Compare
14e80c6 to
5b99c1c
Compare
|
I have adjusted the classes |
5b99c1c to
6a15879
Compare
lippserd
left a comment
There was a problem hiding this comment.
Both concerns regarding the radio element also apply to the select element.
8f31882 to
a350cc5
Compare
491328a to
c26ebdc
Compare
lippserd
left a comment
There was a problem hiding this comment.
Since explicit is better than implicit, I also suggest adding new assertNull() calls after the setValue() calls at the end of testNullAndTheEmptyStringValuesAreEquallyHandled() for both Radio and SelectElementTest. If I'm not mistaken, there is no check to validate if setValue(''|null) leads to getValue(): null.
Since PHP 8.4 implicitly nullable parameter types are deprecated.
Since we require PHP >= 8.2 and since calling this method has no effect as of PHP 8.1.0, as all properties are accessible by default, it is safe to simply remove this method call.
Since PHP 8.5 using `null` as an array offset is deprecated. Code relying on `null` being silently converted to an empty string must be replaced and discouraged in PHPDocs.
- (Radio|Select)Option: Remove support for `null` value by defining the parameter types for the constructor - Ensure empty-string `''` option cannot be `disabled|checked|selected` by falsy values except `null` and `''` - Adjust tests accordingly and remove redundant tests
c26ebdc to
cd48985
Compare
Changes made:
PHP 8.4:
PHP 8.5:
ReflectionProperty::setAccesible()Other changes:
guzzlehttp/psr7resolves #181