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

[Filesystem] Support resources and deprecate using arrays in dumpFile() and appendToFile() #29661

Merged
merged 1 commit into from Mar 3, 2019

Conversation

Projects
None yet
6 participants
@thewilkybarkid
Copy link
Contributor

commented Dec 21, 2018

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Running PHPStan on my project picked up that passing a resource to Filesystem::dumpFile() didn't match the documented type.

I found this has been discussed in #20980 and #28019, without a clear result. But, my reading is that only strings should be supported. While I think that not supporting streams makes this a lot less useful (and I'm going to switch away from it), this does need to be resolved. So, I've deprecated using arrays and resources.

@thewilkybarkid thewilkybarkid changed the title Deprecate using arrays and resources in Filesystem's dumpFile() and appendToFile() [Filesystem] Deprecate using arrays and resources in dumpFile() and appendToFile() Dec 21, 2018

@fabpot

fabpot approved these changes Dec 23, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Dec 24, 2018

@chalasr
Copy link
Member

left a comment

UPGADE-4.3.md needs to be updated

@thewilkybarkid

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Would need a separate PR, but would you accept a new method that takes a resource?

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

What about reusing the same method?

@thewilkybarkid

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

That would be my preference, but this PR was following #20980 and #28019.

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

I would be in favor of supporting resources and strings in the existing method.

@thewilkybarkid thewilkybarkid changed the title [Filesystem] Deprecate using arrays and resources in dumpFile() and appendToFile() [Filesystem] Support resources and deprecate using arrays in dumpFile() and appendToFile() Feb 22, 2019

@thewilkybarkid

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@fabpot Updated to match.

@fabpot

fabpot approved these changes Mar 3, 2019

@fabpot fabpot force-pushed the thewilkybarkid:dump-file-type branch from 33d5f67 to 0eaf9d2 Mar 3, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Thank you @thewilkybarkid.

@fabpot fabpot merged commit 0eaf9d2 into symfony:master Mar 3, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 3, 2019

feature #29661 [Filesystem] Support resources and deprecate using arr…
…ays in dumpFile() and appendToFile() (thewilkybarkid)

This PR was squashed before being merged into the 4.3-dev branch (closes #29661).

Discussion
----------

[Filesystem] Support resources and deprecate using arrays in dumpFile() and appendToFile()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Running PHPStan on my project picked up that passing a resource to `Filesystem::dumpFile()` didn't match the documented type.

I found this has been discussed in #20980 and #28019, without a clear result. But, my reading is that only strings should be supported. While I think that not supporting streams makes this a lot less useful (and I'm going to switch away from it), this does need to be resolved. So, I've deprecated using arrays and resources.

Commits
-------

0eaf9d2 [Filesystem] Support resources and deprecate using arrays in dumpFile() and appendToFile()

@thewilkybarkid thewilkybarkid deleted the thewilkybarkid:dump-file-type branch Mar 4, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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.