Skip to content

More curl tests - #23333

Open
Sjord wants to merge 11 commits into
php:masterfrom
Sjord:more-curl-tests
Open

More curl tests#23333
Sjord wants to merge 11 commits into
php:masterfrom
Sjord:more-curl-tests

Conversation

@Sjord

@Sjord Sjord commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@Sjord
Sjord marked this pull request as ready for review August 18, 2026 08:38
@Sjord
Sjord requested a review from adoy as a code owner August 18, 2026 08:38
@Sjord

Sjord commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

@NickSdot Could you take a look at this?

@NickSdot NickSdot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cannot really judge in detail what's tested, but found some things worth pointing out. Also, can confirm that this closes coverage gaps. ✌️

Coverage: ext/curl

Base: c621cbe27ffe59ed98228724d674dcadcf7f6e8d upstream/master
Tree: 2aa80efcacc776c87712a9beed9487c40be6cffc working tree

+--------+-------+---------+--------------------+-------------------+--------+---------+
|        | Tests | Sources |              Lines |          Branches |   Time |  Memory |
+--------+-------+---------+--------------------+-------------------+--------+---------+
| Base   |   189 |       6 | 2520/2912 (86.54%) | 847/4640 (18.25%) | 64.23s | 45.5 MB |
| Tree   |   197 |       6 | 2538/2912 (87.16%) | 857/4640 (18.47%) | 68.17s | 45.1 MB |
| Change |    +8 |       0 |  +18 / -0 (+0.62%) | +10 / -0 (+0.22%) | +3.94s | -0.4 MB |
+--------+-------+---------+--------------------+-------------------+--------+---------+

Comment on lines +22 to +23
echo $headers;
echo "\n---\n";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could be one echo

}

echo "default 301: ";
echo do_redirect(301), PHP_EOL;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PHP_EOL -> \n everywhere.

Alternatively, maybe you want to make the test less noisy by doing something like:

function do_redirect(string $label, int $code, ?int $postredir = null): void
{
    global $host;
    // curl stuff
    echo $label, ': ', trim(curl_exec($ch)), "\n";
}

foreach ([
    ['default 303', 303],
    ['301 on, code 301', 301, CURL_REDIR_POST_301],
    ['301 on, code 302', 302, CURL_REDIR_POST_301],
] as $case) {
    do_redirect(...$case);
}


// GitHub doesn't actually support SFTP, it does get far enough that the host key callback is called.
$ch = curl_init('sftp://php@github.com/file.txt');
curl_setopt($ch, CURLOPT_SSH_HOSTKEYFUNCTION, 'hostkeyfunction');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure what's up there, but this test says CURLOPT_SSH_HOSTKEYFUNCTION cannot be properly tested. But others do test it; looks like it needs at least a version gate in SKIPIF?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added the version check.

@Girgias Do you have any insight into why CURLOPT_SSH_HOSTKEYFUNCTION cannot be properly tested?

$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, "{$host}/get.inc");
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_MAXFILESIZE_LARGE, 10);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Version gate in SKIPIF?

php-src/ext/curl/curl.stub.php

Lines 1449 to 1454 in c621cbe

/* Available since 7.11.1 */
/**
* @var int
* @cvalue CURLOPT_MAXFILESIZE_LARGE
*/
const CURLOPT_MAXFILESIZE_LARGE = UNKNOWN;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The minimum curl version PHP can be built with is 7.61, so CURLOPT_MAXFILESIZE_LARGE is always available.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But then the responder needs a case for this test that sends content length.

Comment on lines +25 to +26
echo $header_contents;
echo "\n";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could be one echo

echo $header_contents;
echo "\n";

unlink($header_file);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be in --CLEAN--, eg:

--CLEAN--
<?php @unlink(sys_get_temp_dir() . '/curl_setopt_CURLOPT_WRITEHEADER.tmp'); ?>

requires to drop tempnam from $header_file, too.

--FILE--
$header_file = sys_get_temp_dir() . '/curl_setopt_CURLOPT_WRITEHEADER.tmp';

Comment on lines +32 to +39
echo curl_exec($ch);

var_dump(in_array('README.*', $seen_patterns));
var_dump(in_array('README.html', $seen_fnames));

?>
--EXPECT--
The list of Debian mirror sites is available here: https://www.debian.org/mirror/list

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo curl_exec($ch);
var_dump(in_array('README.*', $seen_patterns));
var_dump(in_array('README.html', $seen_fnames));
?>
--EXPECT--
The list of Debian mirror sites is available here: https://www.debian.org/mirror/list
var_dump(curl_exec($ch) !== false);
var_dump(in_array('README.*', $seen_patterns));
var_dump(in_array('README.html', $seen_fnames));
?>
--EXPECT--
bool(true)

We probably do not want changing content to affect tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that this is a little bit brittle, but this is to test that the return value of fnmatch_function works. I.e. fnmatch_function returns that only README.mirrors.txt matches and no other files, so here we expect only the contents of README.mirrors.txt.

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.

2 participants