Skip to content

Fix accepted types for fputcsv fields parameter#451

Merged
ondrejmirtes merged 4 commits intophpstan:masterfrom
DaveLiddament:fix/fputcsv-fields-parameter
Feb 18, 2021
Merged

Fix accepted types for fputcsv fields parameter#451
ondrejmirtes merged 4 commits intophpstan:masterfrom
DaveLiddament:fix/fputcsv-fields-parameter

Conversation

@DaveLiddament
Copy link
Copy Markdown
Contributor

@DaveLiddament DaveLiddament commented Feb 15, 2021

Correctly fix type for fputcsv function parameter $fields. Fix attempt made in #448

  • Update code
  • Add tests
  • Fix failing test

public function testPutCsvWithStringable(): void
{
if (PHP_VERSION_ID < 80000) {
$this->markTestSkipped('Test skipped under 8.0');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I read "under 8.0" as "when running 8.0". Something like "before" or "on lower version than" would be nicer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a better message.

@@ -0,0 +1,17 @@
<?php declare(strict_types = 1);

class StringablePerson implements \Stringable {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't put these test files in global namespace. Some namespace like "namespace FputcsvPhp8" would work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

@@ -0,0 +1,38 @@
<?php declare(strict_types=1);

class Person
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't put these test files in global namespace. Some namespace like "namespace FputcsvTest" would work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

@ondrejmirtes ondrejmirtes merged commit fe4c770 into phpstan:master Feb 18, 2021
@ondrejmirtes
Copy link
Copy Markdown
Member

Thank you!

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