PHP 8.1 deprecated support for null values in its APIs #561
Conversation
Codecov Report
@@ Coverage Diff @@
## master #561 +/- ##
=========================================
Coverage 98.34% 98.34%
- Complexity 1888 1891 +3
=========================================
Files 71 71
Lines 4533 4534 +1
=========================================
+ Hits 4458 4459 +1
Misses 75 75
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Looks reasonable.
@staabm what should we do about unit tests for this sort of stuff?
Do we try to engineer a unit test scenario where $child->group is null and demonstrate that it "fails" on PHP 8.1 with a deprecation notice?
lib/Component.php
Outdated
| @@ -238,7 +238,7 @@ public function select($name) | |||
| return array_filter( | |||
| $result, | |||
| function ($child) use ($group) { | |||
| return $child instanceof Property && strtoupper($child->group) === $group; | |||
| return $child instanceof Property && strtoupper((string) $child->group) === $group; | |||
There was a problem hiding this comment.
I would prefer an explicit null check instead of a cast
There was a problem hiding this comment.
it would also be usefull if we could come up with a more fine grained type then array for the children property at class-level
There was a problem hiding this comment.
I actually used the cast because this is also done in other places in the existing code and IMO it's the best readable option when still needing to support PHP 5.5 as you intend to by the README. Anyway, I changed the code to match your preference.
I also adapted the array type, also I don't see how it helps here. What could help is declaring the $group property as nullable in the Property class, but phpstan does not seem to care, maybe because it is set to a very basic level.
IMO this should be covered by static analysis. so we e.g. might increase the phpstan level until it reports this kind of errors |
phil-davis
dismissed
their stale review
see comments by others - we can wait for some discussion
|
If you enabled reporting of |
|
There are couple of minor code-style things that cs-fixer is complaining about. |
|
Hello, I believe I fixed all of your findings. If there’s still something open you are waiting for please let me know. |
|
Hello, just in case anyone cares, it has been kind of frustrating experience for me trying to contribute to the sabre projects. At this point, I have three open PRs which have stayed without feedback for a long time, in part for years, or like this one where I was first asked to fix style violations and stuff just to be ignored after having done all that. Now that my CI system has also moved to PHP 8.1, I need to use a patched version of Baikal because my tests fail as a result of the issue fixed in this PR. As you claim to support PHP8 and it is becoming more and more widespread, I have no clue why you show no interested in fixing these simple issues. Anyway, I have to say I am pretty discouraged from submitting any more contributions to the sabre projects after these experiences. Which is a shame given that sabre is the leading open source DAV server implementation. |
|
I can totally see that the process is frustrating. We have a similar problem a lot other OSS projects have. We don't have full time maintainers and everyone involved is doing the best he/she can in freetime. If your business depends on the project you should consider supporting the people working on it, so we can afford spending more time on it. |
|
My business doesn’t. My OSS project (a carddav client) does in that I attempt to verify its interoperability with sabre dav. Contributing fixes to problems I discover is the way I can support you, at least that’s what I thought. |
|
@mstilkerich I am working on sabre-io/xml#215 right now. I just released a Once I think that is working OK, I will move on to the other repos, starting with the ones that do not depend on too much other stuff! And I should be able to get your PR sorted out and released as a patch version, before doing bigger changes here in vobject. You code should run OK across all the currently-supported PHP versions 7.1 through 8.1 |
|
@mstilkerich can you rebase this PR and sort out the conflict that GitHub is reporting please. |
Do not pass a null value to strtoupper.
This will throw errors when deprecated PHP features are used.
It (indirectly) writes to stdout by itself.
mstilkerich
force-pushed
the
fix_php81_abquery_groupfilter
branch
from
fb706cf
to
13f3df9
Compare
|
Thank you. I rebased. The conflicting line was already changed to essentially fix the same issue, but I believe it was not correctly fixed. A filter for "." should match any property without a group according to the description of the function, but with the current master it will not return anything. I also added a test case to show this. |
|
Thank you! |
|
@mstilkerich thanks - I will look through other open PRs this evening and see what is an easy-win fix and merge those, then publish a release, hopefully in a few hours. |
When doing an addressbook query report (using Baikal, but I guess it won't matter) on PHP 8.1, PHP raises deprecation notices because strtoupper is called with a null value. This is fixed in this PR.