Skip to content

Build/Test Tools: Mock external HTTP requests in wp_crop_image() URL tests - #14020

Open
huzaifaalmesbah wants to merge 2 commits into
WordPress:trunkfrom
huzaifaalmesbah:tests/mock-crop-image-url
Open

huzaifaalmesbah wants to merge 2 commits into
WordPress:trunkfrom
huzaifaalmesbah:tests/mock-crop-image-url

Conversation

@huzaifaalmesbah

Copy link
Copy Markdown
Member

Description

In tests/phpunit/tests/image/functions.php, test_wp_crop_image_with_url() was annotated with @group external-http because it performed a live HTTP request to https://s.w.org/screenshots/3.9/dashboard.png via PHP's stream wrapper during image loading. Additionally, test_wp_crop_image_should_fail_with_wp_error_object_if_url_does_not_exist() made an unmocked live request to wordpress.org.

This PR replaces the live network requests by mocking the https stream wrapper with Core's WP_Test_Stream in-memory fixture stream, removing live network dependencies and removing @group external-http from test_wp_crop_image_with_url().

Testing Instructions

  1. Run single-site PHPUnit tests for the image cropping methods:
    npm run test:php -- --filter test_wp_crop_image
  2. Verify that the tests run and pass without requiring --group external-http:
    npm run test:php -- --filter test_wp_crop_image_with_url
  3. Run multisite tests:
    npm run test:php -- --filter test_wp_crop_image -c tests/phpunit/multisite.xml

Trac ticket: https://core.trac.wordpress.org/ticket/63914

Use of AI Tools

AI assistance: Yes
Tool(s): Google Antigravity
Model(s): Gemini 3.8 Flash
Used for: Brainstorming stream wrapper mocking with WP_Test_Stream and drafting test boilerplate; test architecture, assertions, and local PHPUnit verification were written and validated by me.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props huzaifaalmesbah, lancewillett.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Without OpenSSL, wrapper setup and cleanup emit warnings and can leak the mock HTTPS wrapper.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Replaces live HTTP dependencies in wp_crop_image() URL tests with an in-memory stream fixture.

Changes:

  • Registers WP_Test_Stream as an HTTPS mock.
  • Removes external HTTP and OpenSSL test requirements.
  • Adds stream-wrapper cleanup.
File Description
tests/​phpunit/​tests/​image/​functions.php Mocks image URLs and restores stream state after tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/phpunit/tests/image/functions.php Outdated
Comment on lines +32 to +34
if ( ! in_array( 'https', stream_get_wrappers(), true ) ) {
stream_wrapper_restore( 'https' );
}
@lancewillett

Copy link
Copy Markdown
Member

Four changes needed before commit:

  • Missing-URL test hits a different error under Imagick. WP_Test_Stream::stream_open() always returns true, so the unregistered URL reads as an empty string. GD returns error_loading_image; Imagick returns invalid_image ("Zero size image string passed"). Use a WP_Test_Stream subclass whose stream_open() returns false when no data is registered, and assert the error_loading_image code.
  • Success test skips the URL path under GD. is_file() is true on the mocked URL and false on a real HTTPS URL, so the https?:// allowance in WP_Image_Editor_GD::load() is never needed. Have the same subclass return false from url_stat().
  • Remove the new tear_down(). Once the mock is registered, https is in stream_get_wrappers(), so the guard never restores a leaked wrapper. The try/finally blocks already clean up.
  • Keep @requires extension openssl on both tests. Without a built-in https wrapper, stream_wrapper_unregister() and stream_wrapper_restore() emit warnings, which fail the tests. They skipped before.

AI review · claude-opus-5-5

@lancewillett lancewillett left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes requested: see the four items in #14020 (comment)

AI review · claude-opus-5-5

@huzaifaalmesbah

Copy link
Copy Markdown
Member Author

Thanks @lancewillett for the thorough review and guidance!

Updated in 3b301a0:

  1. Deterministic missing URL error code: Added WP_Test_Stream_Http_Mock extends WP_Test_Stream whose stream_open() returns false when no data is registered for the requested path, ensuring both GD and Imagick consistently return error_loading_image.
  2. Accurate URL handling in GD: Overrode url_stat() to return false, matching real HTTP/HTTPS stream behavior so is_file() returns false and GD follows its URL path.
  3. Removed tear_down(): Let the try/finally blocks handle restoring the wrapper cleanly.
  4. Restored @requires extension openssl: Kept @requires extension openssl on both URL test methods so they skip cleanly in environments without OpenSSL without emitting warnings.

Verified locally with PHPCS and PHPUnit across both single-site and multisite suites.

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.

3 participants