Skip to content

[PHP 8.1] Add CURLStringFile polyfill #334

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

Merged
merged 2 commits into from
Jan 26, 2023
Merged

[PHP 8.1] Add CURLStringFile polyfill #334

merged 2 commits into from
Jan 26, 2023

Conversation

Ayesh
Copy link
Contributor

@Ayesh Ayesh commented Feb 17, 2021

PHP 8.1 adds a new class CURLStringFile, that works similar to CURLFile, but does not require a path to a file, and accepts a string of file contents instead. This can be polyfilled with a CURLFile sub class that uses a data:// URI inside a constructure and calling CURLFile::__construct with that data URI.

Closes #333.

I will leave this as a draft because I'm not sure how to write tests for this. I'm thinking a PHP CLI server that echoes $_FILES, and a test that checks the returned content for file(s) that it uploaded.

@nicolas-grekas
Copy link
Member

If we add a test case (as you, I don't know which one exactly), it should with the native one and with the polyfilled one (when curl is not installed), so that we can prove that this is really a polyfill.

@derrabus
Copy link
Member

I'm thinking a PHP CLI server that echoes $_FILES, and a test that checks the returned content for file(s) that it uploaded.

Sounds like a plan. You can find an example of using the CLI webserver in tests here:

https://github.com/symfony/symfony/blob/6ae59a9a171695ddef5294898681c060e8c1f3fe/src/Symfony/Component/HttpFoundation/Tests/ResponseFunctionalTest.php#L22-L29

@Ayesh
Copy link
Contributor Author

Ayesh commented Feb 17, 2021

Thanks a lot for the feedback.

I totally forgot about the CURLStringFile::$data mutability, and like @nicolas-grekas said, we might (unfortunately) need to use magic methods here. I updated with __get, __set and __isset methods to update the CURLStringFile::$name if $data was modified. It feels a bit... yucky... to do this, but I'm looking for any suggestions.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

can you please also add a line in the changelog? done for you

@Ayesh
Copy link
Contributor Author

Ayesh commented Mar 12, 2021

Thank you so much for the help @nicolas-grekas - I have made the changes.
I will also push a test. It passes on PHP >= 7.4, and I'm still pulling my hair on why it fails on older PHP versions.

@stof
Copy link
Member

stof commented Mar 12, 2021

I will also push a test. It passes on PHP >= 7.4, and I'm still pulling my hair on why it fails on older PHP versions.

that's because your implementation relies on the fact that curl knows how to deal with a CurlFile object. But that CurlFile won't exist there.

@Ayesh
Copy link
Contributor Author

Ayesh commented Mar 12, 2021

Thank you @stof. Looking at CURLFile doc, it says CURLFile should available in PHP >= 5.5. My direction is that the CLI server failed to start at all in older PHP versions.

@Ayesh
Copy link
Contributor Author

Ayesh commented Mar 12, 2021

I tested with a PHP 7.3 setup, it indeed turns out that the problem is that CURLFile only supports streams in PHP >= 7.4, just like @stof mentioned.
I updated the tests and CURLStringFile to be only available in PHP >= 7.4, so I can mark this PR for review. All tests pass.

For PHP 7.0 through 7.3, I think we could get it working by temporarily writing the contents to a file, but I'm not sure if the effort is worth it. I will open the PR for review either way, and will happily work on support for older PHP versions if you think it's a worthwhile idea. 🤞🏼

Thank you.

@Ayesh Ayesh marked this pull request as ready for review March 12, 2021 17:10
@nicolas-grekas
Copy link
Member

Can you please rebase?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I fixed my last comments)

@nicolas-grekas nicolas-grekas requested a review from stof April 23, 2021 10:58
@nicolas-grekas
Copy link
Member

@Ayesh can you please rebase and check @stof's comments?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I just rebased and resolved @stof's comments.

@nicolas-grekas
Copy link
Member

Thank you @Ayesh.

@nicolas-grekas nicolas-grekas merged commit 3794858 into symfony:main Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.1 - CURLStringFile
5 participants