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 incorrectly overriden static type-hint in Reader and AbstractCsv #285

Merged
merged 3 commits into from Mar 5, 2018

Conversation

Projects
None yet
2 participants
@Jalle19
Copy link
Contributor

Jalle19 commented Mar 3, 2018

Introduction

The following code produces this Phan error: PhanUndeclaredMethod Call to undeclared method \League\Csv\AbstractCsv::setHeaderOffset:

$csvReader = Reader::createFromPath((string)$this->argument('csvPath'));
$csvReader->setHeaderOffset(0);

Proposal

Remove the static return type-hint from AbstractCsv since sub-classes now override createFromPath().

Backward Incompatible Changes

None

Targeted release version

9.1.2

Open issues

There's some references to this in #266

Jalle19 added some commits Mar 3, 2018

Remove static return type-hint from createFromPath()
The type-hint says "@return static", which means if you call Reader::createFromPath() it should return a Reader, not an AbstractCsv. Without this you can't use any of the methods provided by Reader without getting warnings during static analysis.
@nyamsprod

This comment has been minimized.

Copy link
Member

nyamsprod commented Mar 3, 2018

@Jalle19 thanks for your PR. Indeed when 9.0.0 was released this bug on the return type of all the named constructors methods was introduced. The issue that I have is that your resolution may be considered a BC break by some. If it's not a BC break then you also need to fix the other named constructor return type (ie all the createFrom* methods) ?
Bottom line, if you can convince me that this is not a BC break and if you complete the PR by fixing the other named constructors I'll gladly merge your PR.

@Jalle19

This comment has been minimized.

Copy link
Contributor Author

Jalle19 commented Mar 4, 2018

I guess I'll be fixing the other createFrom methods too then. I have a hard time coming up with a scenario where this would break anything, if I do I can mention it here.

@nyamsprod

This comment has been minimized.

Copy link
Member

nyamsprod commented Mar 4, 2018

@Jalle19 TBH I too don't think it is BC break since removing the return type does not restrict or change the current public API. So IMHO we can move on with your PR once finalized ... unless someone disagree 👍

@Jalle19

This comment has been minimized.

Copy link
Contributor Author

Jalle19 commented Mar 5, 2018

@nyamsprod I removed all the relevant static return type-hints

@nyamsprod

This comment has been minimized.

Copy link
Member

nyamsprod commented Mar 5, 2018

let's merge this .. I'll release a new CSV patch version later in the week if I forgot just ping me here again 👍

@nyamsprod nyamsprod merged commit 2e412a3 into thephpleague:master Mar 5, 2018

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Jalle19

This comment has been minimized.

Copy link
Contributor Author

Jalle19 commented Mar 5, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.