-
Notifications
You must be signed in to change notification settings - Fork 350
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
Conversation
phil-davis
commented
Feb 11, 2021
•
edited
Loading
edited
- 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
- 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
- Increase phpstan to level 1 and add some test coverage to keep codecov happy
@@ -75,6 +75,9 @@ public function patch($data, $rangeType, $offset = null) | |||
$f = fopen($this->path, 'c'); | |||
fseek($f, $offset, SEEK_END); | |||
break; | |||
default: |
There was a problem hiding this comment.
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
)
* If auto-prefix is set to false, the hrefs will be treated as absolute | ||
* and not relative to the servers base uri. | ||
* |
There was a problem hiding this comment.
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([], []), | |||
[], |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
$principal = false; | ||
$principalIndex = null; |
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
This includes code from #1325 and #1328 plus increasing the phpstan level from 0 to 1, as suggested in #1325 (comment) |
fe56232
to
0939a25
Compare
@@ -292,6 +292,8 @@ protected function copyNode(INode $source, ICollection $destinationParent, $dest | |||
$destinationName = $source->getName(); | |||
} | |||
|
|||
$destination = null; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm