Make WordPress Core

Opened 9 years ago

Last modified 2 months ago

#42064 new defect (bug)

wp_crop_image() does not work when fopen() is disabled

Reported by: jadonn Owned by:
Priority: normal Milestone: Awaiting Review
Component: Build/Test Tools Version: 4.9
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

While running the WordPress PHP Unit testing framework, a single test for wp_crop_image() failed consistently. After @danielbachhuber debugged the test, he found that wp_crop_image() was failing because it depended on _load_image_to_edit_path, which he said is dependent on fopen(). WordPress is supposed to function when fopen() is disabled. wp_crop_image's dependency on fopen is incorrect.

To reproduce the issue:
Disable allow_url_fopen in PHP
Setup the PHPUnit Test Runner (​https://github.com/WordPress/phpunit-test-runner)
Run the PHPUnit Test Runner.

The test runner should report the following failed test:
Tests_Image_Functions::test_wp_crop_image_url Failed asserting that WP_Error Object (...) is not an instance of class "WP_Error". /../../../phpunit-test-runner/wp-test-runner/tests/phpunit/tests/image/functions.php:317

The WP_Error object reports the following:
Fobject(WP_Error)#9993 (2) {

errors=>
array(1) {

invalid_image=>
array(1) {

[0]=>
string(21) "File is not an image."

}

}
error_data=>
array(1) {

invalid_image=>
string(60) "​https://asdftestblog1.files.wordpress.com/2008/04/canola.jpg"

}

}

The image is a valid image otherwise, such as when viewed through the browser. The test fails on PHP 5.6, PHP 7.0, and PHP 7.1 when fopen is disabled.

Change History (6)

This ticket was mentioned in ​Slack in #hosting-community by mike. ​View the logs.


9 years ago

#2 @danielbachhuber
9 years ago

Related: https://core.trac.wordpress.org/ticket/37681#comment:1

It appears to be hit if you're editing an image which was uploaded pre-2.7 and/or if the file metadata has gone missing. In that case, the image editors will treat the image URL as the source image through the filesystem wrappers, bypassing wp_remote_get(), which is why the fopen and allow_url_fopen are there.

Originally committed in r20384

I see a couple of options:

  1. Use download_url() to download a copy of the image to /tmp, and permit image modifications from that.
    • Advantage: Greater cross-platform compatibility for this feature.
    • Disadvantage: Web server has a potentially duplicate copy of the image on the filesystem (which is likely inconsequential).
  2. Mark Tests_Image_Functions::test_wp_crop_image_url as skipped when fopen isn't available or allow_url_fopen is false.
    • Advantage: Least risky approach.
    • Disadvantage: Adds yet another skipped test to the test suite.

This ticket was mentioned in ​Slack in #hosting-community by mike. ​View the logs.


9 years ago

#4 @Hareesh Pillai
5 years ago

  • Component General → Build/Test Tools

This ticket was mentioned in ​PR #12751 on ​WordPress/wordpress-develop by ​@arkaprabhachowdhury.


3 months ago
#5

  • Keywords has-patch has-unit-tests added

## Summary

  • download remote crop sources through the WordPress HTTP API
  • remove the temporary source file after the image editor loads it
  • replace the external-network crop test with a deterministic HTTP mock

## Testing

  • PHP lint for both changed files
  • PHPCBF and PHPCS for both changed files
  • focused PHPUnit with allow_url_fopen=0: 1 test, 4 assertions
  • Tests_Image_Functions: 113 tests, 211 assertions

#6 @arkaprabhachowdhury
2 months ago

Opened [PR #12751](​https://github.com/WordPress/wordpress-develop/pull/12751) to address this ticket. Summary - download remote crop sources through the WordPress HTTP API - remove the temporary source file after the image editor loads it - replace the external-network crop test with a deterministic HTTP mock Testing - PHP lint for both changed files - PHPCBF and PHPCS for both changed files - focused PHPUnit with allow_url_fopen=0: 1 test, 4 assertions - Tests_Image_Functions: 113 tests, 211 assertions The PR includes focused regression coverage, and its validation details are recorded in the PR description.

I picked this up because the ticket describes a concrete core behavior or edge case that remains relevant in current WordPress. The proposed change is intentionally scoped to the ticket and is submitted here for code review, with the technical discussion remaining on this ticket.

Note: See TracTickets for help on using tickets.