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

phpstan level 1 and other fixes #1329

Merged
merged 3 commits into from
Feb 11, 2021
Merged

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Feb 11, 2021

  1. Correctly process a POST with no Content-Type specified - code from PR Correctly process a POST with no Content-Type specified #1325 and issue Trying to request dav endpoint with POST request gives 500 Internal Server Error #1324
  2. Test code is not part of vobject distribution - do not run vobject tests in CI - code from PR Test code is not part of vobject distribution - do not run vobject tests in CI #1328 and issue CI failing due to no vobject test code #1327
  3. Increase phpstan to level 1 and add some test coverage to keep codecov happy

@phil-davis phil-davis self-assigned this Feb 11, 2021
@@ -75,6 +75,9 @@ public function patch($data, $rangeType, $offset = null)
$f = fopen($this->path, 'c');
fseek($f, $offset, SEEK_END);
break;
default:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpstan reported that $f might not be defined below.
If the caller passes something other than 1 2 or 3 what should we do?
(I made it do gthe same thing as if they passed in 1)

Comment on lines -42 to -44
* If auto-prefix is set to false, the hrefs will be treated as absolute
* and not relative to the servers base uri.
*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The constructor had the 2nd parameter removed some time ago. So remove the PHPdoc. And fix the old calls in the test code...

@@ -75,7 +75,6 @@ public function testInvalidArg1()
$this->expectException('InvalidArgumentException');
$obj = new SchedulingObject(
new Backend\MockScheduling([], []),
[],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SchedulingObject only takes 2 parameters in the constructor. The test code is out-of-date.

if (null === $pdo) {
$this->markTestSkipped($this->driver.' was not recognised');
}

$pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpstan reported that $pdo might not be set at this point, which is technically possible. Add some code above to deal with that case.

Comment on lines +134 to +135
$principal = false;
$principalIndex = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpstan reports that these might not be defined, so initiialize them just in case the foreach has an empty loop.

@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #1329 (0939a25) into master (4258420) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1329   +/-   ##
=========================================
  Coverage     97.12%   97.12%           
- Complexity     2787     2788    +1     
=========================================
  Files           174      174           
  Lines          8039     8045    +6     
=========================================
+ Hits           7808     7814    +6     
  Misses          231      231           
Impacted Files Coverage Δ Complexity Δ
lib/DAV/Xml/Property/Href.php 100.00% <ø> (ø) 12.00 <0.00> (ø)
lib/CalDAV/Schedule/Plugin.php 99.22% <100.00%> (+<0.01%) 100.00 <0.00> (ø)
lib/DAV/Browser/Plugin.php 87.25% <100.00%> (+0.09%) 85.00 <0.00> (+1.00)
lib/DAV/FSExt/File.php 100.00% <100.00%> (ø) 11.00 <0.00> (ø)
lib/DAV/Tree.php 100.00% <100.00%> (ø) 41.00 <0.00> (ø)

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 4258420...0939a25. Read the comment docs.

@phil-davis phil-davis marked this pull request as ready for review February 11, 2021 06:30
@phil-davis
Copy link
Contributor Author

This includes code from #1325 and #1328 plus increasing the phpstan level from 0 to 1, as suggested in #1325 (comment)

@@ -292,6 +292,8 @@ protected function copyNode(INode $source, ICollection $destinationParent, $dest
$destinationName = $source->getName();
}

$destination = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpstan reports that $destination might not be defined. So set it here, just in case.

Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

Lgtm

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.

None yet

2 participants