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

Add PHP 8.4 compliance for v4 #296

Merged
merged 3 commits into from
Sep 5, 2024
Merged

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Sep 5, 2024

Apply the small code changes and the CI and tooling changes for PHP 8.4 to master (which is where the v4 release series comes from).

And fixup parse() and expect() functions in Service.php - see the comments below about what phpstan noticed.
Now, if someone passes a closed resource, we will throw the usual ParseException that is thrown when someone passes an empty input. Fixes issue #297

@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 5, 2024

 ------ ----------------------------------------------------------------- 
  Line   lib/Service.php                                                  
 ------ ----------------------------------------------------------------- 
  125    Parameter #1 $source of method XMLReader::XML() expects string,  
         resource|string given.                                           
  169    Parameter #1 $source of method XMLReader::XML() expects string,  
         resource|string given.                                           
 ------ ----------------------------------------------------------------- 

Hmmm - phpstan is complaining about this. It did not complain in the older 2.2 and v3 branches. I will check what the code looks like there.

Edit: phpstan is at level 6 in v3. It is at level 8 in master (v4 release series)

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.71%. Comparing base (297bda9) to head (7e41f14).
Report is 5 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #296      +/-   ##
============================================
+ Coverage     96.68%   96.71%   +0.02%     
- Complexity      115      117       +2     
============================================
  Files            13       13              
  Lines           483      487       +4     
============================================
+ Hits            467      471       +4     
  Misses           16       16              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 5, 2024

I raised an issue with phpstan phpstan/phpstan#11634
See the comments there. Actually the problem is in our code.

https://www.php.net/manual/en/function.is-resource.php

Note:
is_resource() is not a strict type-checking method: it will return false if value is a resource variable that has been closed.

When we use is_resource() that actually does "is open resource". The variable could still be a resource, but closed.

https://www.php.net/manual/en/function.stream-get-contents.php
If we call stream_get_contents with a closed resource, it explodes.

I added unit tests, and enhanced the checks, so that we throw the existing ParseException for empty input if someone passes us a closed resource.

After this is reviewed, I will backport it to the v3 and v2 release series. It seems useful to make sure that we handle a closed resource as nicely as we can.

@phil-davis phil-davis marked this pull request as ready for review September 5, 2024 15:14
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.

2 participants