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

Fix return types #1362

Merged
merged 2 commits into from
Nov 3, 2021
Merged

Fix return types #1362

merged 2 commits into from
Nov 3, 2021

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Nov 2, 2021

Fixes #1361

I have tried to minimize changes to real code, and o mostly just delete or adjust PHPdoc to match reality. That gets CI green with the minimum impact.

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #1362 (a8ea6b8) into master (01ba0df) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head a8ea6b8 differs from pull request most recent head cea1483. Consider uploading reports for the commit cea1483 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1362      +/-   ##
============================================
+ Coverage     97.30%   97.31%   +0.01%     
  Complexity     2806     2806              
============================================
  Files           174      174              
  Lines          7975     7975              
============================================
+ Hits           7760     7761       +1     
+ Misses          215      214       -1     
Impacted Files Coverage Δ
lib/CalDAV/Plugin.php 98.17% <ø> (ø)
lib/CalDAV/Schedule/Plugin.php 99.14% <ø> (ø)
lib/CalDAV/Subscriptions/Subscription.php 100.00% <ø> (ø)
lib/DAV/Auth/Plugin.php 100.00% <ø> (ø)
lib/DAV/Browser/Plugin.php 86.03% <ø> (ø)
lib/DAVACL/Plugin.php 95.28% <ø> (ø)
lib/DAVACL/PrincipalBackend/AbstractBackend.php 100.00% <ø> (ø)
lib/DAVACL/Xml/Property/Principal.php 100.00% <100.00%> (+2.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01ba0df...cea1483. Read the comment docs.

@phil-davis
Copy link
Contributor Author

phil-davis commented Nov 2, 2021

------ -------------------------------------------------------- 
  Line   tests/Sabre/CalDAV/ExpandEventsDTSTARTandDTENDTest.php  
 ------ -------------------------------------------------------- 
  100    Variable $vObject might not be defined.                 
 ------ -------------------------------------------------------- 

phpstan fails in CI, but I don't get this fail locally.

CI has phpstan (0.12.99) and I have the same version locally.

Edit: the difference is that locally I am running PHP 7.4 and phpunit 9.5.10. The CI was running PHP 7.1 with phpunit 7 for the code analysis job. The phpunit 7 Assert methods probably do something a bit different and phpstan understands the test code flow a bit differently.

It should be fine to run the code analysis with PHP 7.4

@phil-davis phil-davis marked this pull request as ready for review November 2, 2021 17:58
lib/CalDAV/Plugin.php Outdated Show resolved Hide resolved
@@ -735,9 +735,7 @@ public function outboxRequest(IOutbox $outboxNode, RequestInterface $request, Re

/**
* This method is responsible for parsing a free-busy query request and
* returning it's result.
*
* @return string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function has no return statements at all. Why is the PHPdoc saying that it should return a string?

lib/CalDAV/Subscriptions/Subscription.php Outdated Show resolved Hide resolved
lib/CalDAV/Plugin.php Outdated Show resolved Hide resolved
@@ -140,6 +140,8 @@ public function getCalendarObjects($calendarId)
],
];
}

return [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From here, the changes are test code.
We can put this sort of return at the end of a function to keep phpstan happy. It will never be executed, but that is no problem.

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.

Latest phpstan is reporting missing return statements
3 participants