Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.1 deprecated support for null values in its APIs #561

Merged
merged 10 commits into from
Aug 17, 2022

Conversation

mstilkerich
Copy link
Contributor

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
Copy link

codecov bot commented Jan 14, 2022

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%) ⬆️

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

phil-davis
phil-davis previously approved these changes Jan 14, 2022
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer an explicit null check instead of a cast

Copy link
Member

@staabm staabm Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mstilkerich mstilkerich Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@staabm
Copy link
Member

staabm commented Jan 14, 2022

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

@phil-davis phil-davis self-requested a review January 14, 2022 13:55
@phil-davis phil-davis dismissed their stale review January 14, 2022 13:55

see comments by others - we can wait for some discussion

@mstilkerich
Copy link
Contributor Author

mstilkerich commented Jan 14, 2022

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
Copy link
Contributor

phil-davis commented Jan 14, 2022

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

tests/phpunit.xml.bak Outdated Show resolved Hide resolved
@phil-davis phil-davis self-requested a review January 14, 2022 16:50
@mstilkerich
Copy link
Contributor Author

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

@mstilkerich
Copy link
Contributor Author

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
Copy link
Member

staabm commented Aug 17, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

@mstilkerich I am working on sabre-io/xml#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
Copy link
Contributor

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

@mstilkerich
Copy link
Contributor Author

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.

lib/Cli.php Show resolved Hide resolved
lib/Component.php Outdated Show resolved Hide resolved
@phil-davis phil-davis merged commit 0f625de into sabre-io:master Aug 17, 2022
@mstilkerich
Copy link
Contributor Author

Thank you!

@phil-davis
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants