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.

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

[ext-curl] Add \CURLOPT_INFILESIZE_LARGE #17637

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

OskarStark
Copy link

@OskarStark OskarStark commented Jan 30, 2025

@OskarStark OskarStark force-pushed the feature/CURLOPT_INFILESIZE_LARGE branch from 1637686 to 8678ead Compare January 30, 2025 10:22
@OskarStark OskarStark changed the title [ext-curl] Add à\CURLOPT_INIFILESIZE_LARGE` [ext-curl] Add \CURLOPT_INIFILESIZE_LARGE Jan 30, 2025
This is my first PR on this repository and is based on:
* https://github.com/symfony/symfony/pull/59654/files#r1935358570

So feel free to close it :-)
@OskarStark OskarStark force-pushed the feature/CURLOPT_INFILESIZE_LARGE branch from 8678ead to bbad36c Compare January 30, 2025 10:22
ext/curl/interface.c Outdated Show resolved Hide resolved
@devnexen
Copy link
Member

devnexen commented Jan 30, 2025

Two other things:

1/ you very likely edited curl_arginfo.h manually, I would suggest to revert this change locally and using build/gen_stub.php instead, i.e. php build/gen_stub.php ext/curl/curl.stub.php.
2/ Someone will very likely ask you to add a test before committing your change (i.e. ext/curl/tests for how they are made).

@devnexen devnexen requested a review from Ayesh January 30, 2025 11:23
@OskarStark
Copy link
Author

1/ you very likely edited curl_arginfo.h manually, I would suggest to revert this change locally and using build/gen_stub.php instead, i.e. php build/gen_stub.php ext/curl/curl.stub.php.

done, thanks

@arnaud-lb
Copy link
Member

The existence of two distinct options CURLOPT_INFILESIZE and CURLOPT_INFILESIZE_LARGE is mostly due to low-level C considerations and I'm wondering if this distinction is useful/necessary in PHP. Are there cases where using CURLOPT_INFILESIZE_LARGE instead of CURLOPT_INFILESIZE would not behave as expected? Otherwise, should we define CURLOPT_INFILESIZE to @cvalue CURLOPT_INFILESIZE_LARGE directly?

@OskarStark
Copy link
Author

OskarStark commented Jan 30, 2025

I cannot answer this question, but maybe @nicolas-grekas can, but in the linked PR, we need to behave differently yes

@arnaud-lb
Copy link
Member

I'm seeing just now that we already expose other _LARGE constants, so my suggestion would create inconsistencies. Therefore it's probably better to just add CURLOPT_INFILESIZE_LARGE as you are suggesting.

@TimWolla TimWolla changed the title [ext-curl] Add \CURLOPT_INIFILESIZE_LARGE [ext-curl] Add \CURLOPT_INFILESIZE_LARGE Jan 31, 2025
@TimWolla
Copy link
Member

FWIW: The title of the PR typoed the name of the constant, the same issue also exists in the commit message, so probably be careful when merging this.

@OskarStark
Copy link
Author

Hi @TimWolla can you explain? I can't see the typo

@devnexen
Copy link
Member

devnexen commented Feb 1, 2025

he meant this

Add \CURLOPT_IN I FILESIZE_LARGE
...

you can amend this commit at some point, or the committer will take care of it no worries :)

@OskarStark
Copy link
Author

OskarStark commented Feb 1, 2025

Will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants