[Filesystem] Add test to prevent regression when using array|resource with dumpFile #28019
Conversation
thePanz
commented
Jul 20, 2018
•
| Q | A |
|---|---|
| Branch? | 2.8 |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | none |
| License | MIT |
|
See #20980 (comment) :) |
|
Damn, didn't find that PR before opening this one @ro0NL! Sorry for the duplicated MR! |
|
Should we reconsider since this works? If we really want a string, we should maybe throw a deprecation? That will force us to consider which alternative we should provide to ppl hit by the notice (and maybe make use realize it's simpler to just accept what already works?) |
|
(could you rebase on latest 2.8 please to get rid of the CS fixes?) |
|
@nicolas-grekas Rebased, cleaned-up and pushed |
There was a problem hiding this comment.
yes, I changed my mind: there are two ways here: either "officialize" what already works (this PR), or deprecate what shouldn't work. The least effort is this PR, and the downside (making an implementation that doesn't rely on file_put_contents more complex) hypothetical.
|
Note in 3.4 there's also |
|
I'm 👎 on this change for the reasons expressed in the linked PR. |
|
I reverted the docblock change but kept the test case so that we don't accidentally break BC. 👍 |
|
Thank you @thePanz. |
…rray|resource with dumpFile (thePanz) This PR was merged into the 2.8 branch. Discussion ---------- [Filesystem] Add test to prevent regression when using array|resource with dumpFile | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT Commits ------- db1c21c [Filesystem] Add test to prevent regression when using array|resource with dumpFile
…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()