Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Filesystem] Add test to prevent regression when using array|resource with dumpFile#28019

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:2.8fromthePanz:patch-1
Aug 10, 2018

Conversation

@thePanz
Copy link
Contributor

@thePanzthePanz commentedJul 20, 2018•
edited
Loading

QA
Branch?2.8
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsnone
LicenseMIT

@ro0NL
Copy link
Contributor

See#20980 (comment) :)

@thePanz
Copy link
ContributorAuthor

thePanz commentedJul 20, 2018•
edited
Loading

Damn, didn't find that PR before opening this one@ro0NL! Sorry for the duplicated MR!
So I guess the only way to use aresource withFilesystem::dumpFile() is to copy its code somewhere else :(

@nicolas-grekas
Copy link
Member

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?)

@nicolas-grekasnicolas-grekas added this to the2.8 milestoneJul 23, 2018
@nicolas-grekas
Copy link
Member

(could you rebase on latest 2.8 please to get rid of the CS fixes?)

@thePanz
Copy link
ContributorAuthor

@nicolas-grekas Rebased, cleaned-up and pushed

@thePanzthePanz changed the base branch from4.1 to2.8July 26, 2018 11:17
@thePanzthePanz changed the titleWIP: Filesystem::dumpFile() accepts array|resourceFiilesystem::dumpFile() accepts array|resourceJul 26, 2018
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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.

@ro0NL
Copy link
Contributor

Note in 3.4 there's alsoappendToFile() which should be updated as well IMHO

@fabpot
Copy link
Member

I'm 👎 on this change for the reasons expressed in the linked PR.

@nicolas-grekasnicolas-grekas changed the titleFiilesystem::dumpFile() accepts array|resource[Filesystem] Add test to prevent regression when using array|resource with dumpFileAug 7, 2018
@nicolas-grekas
Copy link
Member

I reverted the docblock change but kept the test case so that we don't accidentally break BC.
Removing support for array|resource would need a deprecation layer.

👍

@nicolas-grekas
Copy link
Member

Thank you@thePanz.

thePanz reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas merged commitdb1c21c intosymfony:2.8Aug 10, 2018
nicolas-grekas added a commit that referenced this pull requestAug 10, 2018
…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       | MITCommits-------db1c21c [Filesystem] Add test to prevent regression when using array|resource with dumpFile
@thePanzthePanz deleted the patch-1 branchAugust 10, 2018 08:05
fabpot added a commit that referenced this pull requestMar 3, 2019
…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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

2.8

Development

Successfully merging this pull request may close these issues.

6 participants

@thePanz@ro0NL@nicolas-grekas@fabpot@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp