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.

Sign up for GitHub

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

Jump to bottom

[Filesystem] Updated phpdoc on allowed types of content #20980

Closed
wants to merge 2 commits into from
Closed

[Filesystem] Updated phpdoc on allowed types of content #20980

wants to merge 2 commits into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Dec 18, 2016

Q A
Branch? 2.7
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20612 (comment)
License MIT
Doc PR symfony/symfony-docs#...

@chalasr
Copy link
Member

chalasr commented Dec 18, 2016

I believe dumpFile does not handle streams before 2.8 (see #16156), maybe this should target it instead of 2.7?

Status: reviewed
👍

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 18, 2016

I think were good... this applies to $content as a stream resource used with file_put_contents.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Dec 26, 2016
@nicolas-grekas
Copy link
Member

Technically, this is right, but this is a contract change. I'm not sure we want to guarantee that it works for anything else but strings.
If we want, then this needs corresponding test cases, and it could target master.

@nicolas-grekas
Copy link
Member

That could even be considered a BC break if we read things strictly.

@xabbuh
Copy link
Member

xabbuh commented Dec 28, 2016

👎 for me. Making this change means that we have to support the other types too. Thus, moving away from file_put_contents() in the future could become harder than necessary.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 28, 2016 •
edited
Loading

Making it a behavior discussion then.. do we allow users to rely on it right now?

edit: this would lead to is_string safety guard... closing :)

@ro0NL ro0NL closed this Dec 28, 2016
@ro0NL ro0NL deleted the patch-1 branch December 28, 2016 16:41
@ro0NL ro0NL mentioned this pull request Jan 6, 2017
Merged
@ro0NL ro0NL mentioned this pull request Jul 20, 2018
Merged
@thewilkybarkid thewilkybarkid mentioned this pull request Dec 21, 2018
Merged
fabpot added a commit that referenced this pull request Mar 3, 2019
@fabpot
feature #29661 [Filesystem] Support resources and deprecate using arr…
1e94c50 
…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()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Filesystem Status: Reviewed
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
5 participants
@ro0NL @chalasr @nicolas-grekas @xabbuh @carsonbot