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

[HttpFoundation] Add support for\SplTempFileObject inBinaryFileResponse#49144

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

Conversation

alexandre-daubois
Copy link
Member

@alexandre-dauboisalexandre-daubois commentedJan 28, 2023
edited
Loading

QA
Branch?6.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#48530
LicenseMIT
Doc PRTodo

Add support for\SplTempFileObject inBinaryFileResponse

OskarStark and asrorbekh reacted with rocket emoji
@carsonbot
Copy link

Hey!

I think@alli83 has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@fabpot
Copy link
Member

Would it be possible to add support inBinaryFileResponse instead?

@fabpotfabpot modified the milestones:6.4,7.1Oct 11, 2023
@alexandre-daubois
Copy link
MemberAuthor

It doesn't seem to.BinaryFileResponse needs a "physical" file. This would mean that the temporary file would have to be physically written to disk each time, and we'd lose the whole point of this new method I guess.

@nicolas-grekas
Copy link
Member

BinaryFileResponse accepts an SplFileInfo as argument and SplTempFileObject extends SplFileInfo, so it's already possible to pass a SplTempFileObject there. Can't we make BinaryFileResponse able to deal with such instances, skipping all the features that need a physical file when this happens?

@alexandre-dauboisalexandre-dauboisforce-pushed thetemp-streamed-response branch 4 times, most recently from6294b4a to0151376CompareOctober 11, 2023 11:12
@alexandre-dauboisalexandre-daubois changed the title[HttpFoundation] AddStreamedResponse::createFromTempFile()[HttpFoundation] Add support for\SplTempFileObject inBinaryFileResponseOct 11, 2023
@alexandre-daubois
Copy link
MemberAuthor

Thank you for the hint Nicolas, I tried a implementation inBinaryFileResponse to add support for\SplTempFileObject

@fabpot
Copy link
Member

@alexandre-daubois Can you rebase on current 7.1? Thank you.

@alexandre-daubois
Copy link
MemberAuthor

Yep of course, here you go 🙂

@fabpot
Copy link
Member

Thank you@alexandre-daubois.

@fabpotfabpot merged commitd9d6024 intosymfony:7.1Feb 5, 2024
@fabpotfabpot mentioned this pull requestMay 2, 2024
nicolas-grekas added a commit that referenced this pull requestNov 5, 2024
…BinaryFileResponse` (elementaire)This PR was merged into the 7.1 branch.Discussion----------[HttpFoundation] Fix support for `\SplTempFileObject` in `BinaryFileResponse`| Q             | A| ------------- | ---| Branch?       | 7.1| Bug fix?      | yes| PR original Feature |#49144| New feature?  | no| Deprecations? | no| Issues        |  -| License       | MITWe can not call some methods with an object of `\SplTempFileObject()`. We get this error: `[...] stat failed for php://temp`.I have checked the code against methods listed in [this note](https://www.php.net/manual/en/class.spltempfileobject.php#128962).I have found:- `getMTime()` called in `BinaryFileResponse::setAutoLastModified()`- `getSize()` called in `BinaryFileResponse::prepare()`- `isReadable()` called in `BinaryFileResponse::setFile()` (already safe)I have updated the unit test and patched the class `BinaryFileResponse`.Note: calling `SplFileObject::fstat()` gives `mtime` equals to `0`. I think it is nonsense to use that as value for last modified. I have decided to use `time()` because i guess, we can not do better. Indeed, we have no idea how much time have passed between making the temp file and the call to `setAutoLastModified()` by the developper.Commits-------a104d50 Fix support for \SplTempFileObject in BinaryFileResponse
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

[BinaryFileResponse] Possible to use SplTempFileObject for download attachments.
4 participants
@alexandre-daubois@carsonbot@fabpot@nicolas-grekas

[8]ページ先頭

©2009-2025 Movatter.jp