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

Added more tests and scrutnizer fixes #107

Merged
merged 12 commits into from Feb 16, 2019
Merged

Added more tests and scrutnizer fixes #107

merged 12 commits into from Feb 16, 2019

Conversation

Nyholm
Copy link
Owner

@Nyholm Nyholm commented Feb 16, 2019

No description provided.

@@ -40,7 +40,7 @@
/** @var bool */
private $moved = false;

/** @var int|null */
/** @var int */
Copy link
Owner Author

Choose a reason for hiding this comment

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

It is actually impossible to create an UploadedFile with $size = null

*
* @throws \RuntimeException on error
*/
private function copyToStream(StreamInterface $source, StreamInterface $dest, $maxLen = -1): void
private function copyToStream(StreamInterface $source, StreamInterface $dest): void
Copy link
Owner Author

Choose a reason for hiding this comment

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

We are never using the third parameter, That is why I could remove the dead code

@@ -19,7 +19,7 @@ public function testConstructorInitializesProperties()
$this->assertTrue($stream->isWritable());
$this->assertTrue($stream->isSeekable());
$this->assertEquals('php://temp', $stream->getMetadata('uri'));
$this->assertInternalType('array', $stream->getMetadata());
$this->assertIsArray($stream->getMetadata());
Copy link
Owner Author

Choose a reason for hiding this comment

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

assertInternalType is deprecated

@Nyholm Nyholm mentioned this pull request Feb 16, 2019
@Nyholm
Copy link
Owner Author

Nyholm commented Feb 16, 2019

Im done with this PR now.

@Zegnat
Copy link
Collaborator

Zegnat commented Feb 16, 2019

I don’t understand the Travis error on this one… It seems to complain about MessageTrait but I don’t see you changing MessageTrait?

I haven’t used PHPStan before so this will be interesting to check out. Overall all the code changes look good to me!

@Nyholm
Copy link
Owner Author

Nyholm commented Feb 16, 2019

PHPStan is pretty cool. We do get a few false negatives but that is fine.

I added the docs on MessageTrait::$stream to allow null. We already did allow it but it was not documented. That is the "BC break".

It is not an actual BC break for two reasons:

  • It is a private property on our "internal trait"
  • The behavior of the code was never changed.

Thank you for the review

@Nyholm Nyholm merged commit 3c4d8e5 into master Feb 16, 2019
@Nyholm Nyholm deleted the patch-scrutinizer branch February 16, 2019 16:57
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