PHP 8.1 deprecated support for null values in its APIs

[mstilkerich]

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.

[codecov[bot]]

Codecov Report

Merging #561 (63efe38) into master (833743d) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@            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           

Impacted Files	Coverage Δ
lib/Property.php	98.99% <ø> (ø)
lib/Cli.php	98.07% <100.00%> (ø)
lib/Component.php	99.15% <100.00%> (ø)
lib/Parameter.php	99.24% <100.00%> (ø)
lib/Property/Text.php	99.13% <100.00%> (+<0.01%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[staabm]

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?

IMO this should be covered by static analysis. so we e.g. might increase the phpstan level until it reports this kind of errors

[mstilkerich]

If you enabled reporting of E_DEPRECATED errors when running the unit tests, these error would have shown up. I did so and fixed the other reported passing of null to PHP APIs where a string is expected.

[phil-davis]

There are couple of minor code-style things that cs-fixer is complaining about.

[mstilkerich]

Hello, I believe I fixed all of your findings. If there’s still something open you are waiting for please let me know.

[mstilkerich]

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.

[staabm]

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.

[mstilkerich]

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.

[phil-davis]

@mstilkerich I am working on https://github.com/sabre-io/xml/issues/215 right now. I just released a sabre/uri that has all PHP types declared (there was not much to do there). I am about to release a major version of sabre/xml that requires a minimum of PHP 7.4 and has type declarations everywhere.

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

[phil-davis]

@mstilkerich can you rebase this PR and sort out the conflict that GitHub is reporting please.

[mstilkerich]

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.

[mstilkerich]

Thank you!

[phil-davis]

@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.