Make WordPress Core

Opened 6 years ago

Last modified 33 hours ago

#49631 assigned task (blessed)

Test coverage for media_sideload_image() _source_url meta

Reported by: antpb Owned by: killua99
Priority: normal Milestone: Future Release
Component: Media Version: 5.4
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

Since #48164 when an image is sideloaded we store the original attachment URL in _source_url. There should be test coverage across this feature that validates that the meta is being properly stored.

@pbiron mentioned in the Media meeting where this was discussed that there are currently no existing tests around the media_sideload_image() function. This would be a great start to getting that covered.

@joemcgill also mentioned that "You may want to consider writing the test in a way that sideloads an image that is already available on the test site."

Link to media meeting where this took place: https://wordpress.slack.com/archives/C02SX62S6/p1584022312111000

Change History (17)

#1 @antpb
6 years ago

  • Summary Test coverage forTest coverage for media_sideload_image() _source_url meta

#2 @joemcgill
6 years ago

  • Owner set to killua99
  • Status newassigned

Assigning to @killua99 for a first pass.

Note that you can write a test using WP_TESTS_DOMAIN to sideload from the site set up during testing, rather than an external source.

This ticket was mentioned in Slack in #core by killua99. View the logs.


6 years ago

#4 @killua99
6 years ago

  • Keywords has-patch added; needs-patch removed

I wrote the first pass.

The source is not coming from the site setup since is a mockup with http://example.org/ is hard to sideload even creating the attachment upload, the request to download fails and is impossible to test it in that way. I found something that could be useful for now. Perhaps add some TODO notes on the test to improve the real sideload from a mockup site that could be accessible on travis.

This ticket was mentioned in Slack in #core-media by killua99. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-restapi by killua99. View the logs.


6 years ago

#9 @killua99
6 years ago

Ok now the latest patch has a full mockup to the HTTP request. No real external assets is been use rather than the DIR_TESTDATA and that's ok. Hope this test cover the scope (plus some extras)

This ticket was mentioned in Slack in #core by whyisjake. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 years ago

#12 @antpb
6 years ago

  • Milestone 5.55.6

Hello! This was discussed in the recent Media Component meeting. We are currently in Beta 1 for 5.5 and might need to move this to the 5.6 milestone. If anyone wants to finish this up, I think it's fair to include in the next beta cycle but we probably don't want to go too far into the future beta releases.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 years ago

#14 @antpb
6 years ago

  • Milestone 5.6Future Release

Hello @killua99 ! Thanks for the work on these tests! In the recent Media meeting, the team agreed we should mark this for Future Release. If you think this can be finished before Beta 2 of 5.6 please feel free to let us know and we'll move it back into the milestone. :)

#15 @antpb
6 years ago

  • Type defect (bug)task (blessed)

This ticket was mentioned in PR #13397 on WordPress/wordpress-develop by @mukesh27.


33 hours ago
#16

  • Keywords has-unit-tests added

Adds unit test coverage for media_sideload_image(), which has none today. This picks up where #190 left off and addresses the review feedback left there.

### What is covered

The tests live in the existing Tests_Admin_IncludesMedia test case (tests/phpunit/tests/admin/includesMedia.php) rather than a new file, since that is where the rest of the wp-admin/includes/media.php coverage already sits.

  • _source_url post meta (the ticket's original focus, added in [47251] / #48164), including that the meta stores the URL as passed when it carries a query string, while the file name has the query string stripped.
  • Every $return_type, via a data provider: the default image tag, 'html', 'src', 'id', and an unrecognized value falling back to the image tag.
  • $desc being used as escaped alt text on the returned tag.
  • Attachment parentage — the attachment is attached to the given post.
  • Invalid URLs — a disallowed extension, an executable extension, no extension, and a partial extension (.jpgx, exercising the \b in the regex) each return an image_sideload_failed / "Invalid image URL." error *before* any HTTP request is made. An empty $file is covered separately.
  • The image_sideload_extensions filter — adding an extension (bmp) lets a matching URL through and still records _source_url, removing jpg/jpeg/jpe rejects one, and the filter receives the URL being sideloaded as its second argument.
  • Download failures — a transport WP_Error and a non-200 response are both returned to the caller, and no attachment is created.

### Addressing the feedback on #190

  • "This teardown is not needed"wpTearDownAfterClass() is gone; posts are created per test with the factory and rolled back automatically. tear_down() now only calls remove_added_uploads(), which *is* still required (WP_UnitTestCase snapshots the uploads directory in set_up() but does not clear it), and matches Tests_Post_Attachments, Tests_Image_SiteIcon and others. The remove_filter() call is dropped — hooks are restored between tests.
  • "This would need to return more than just a response code" — the pre_http_request mock now returns a complete response array (headers, body, response, cookies, filename) so download_url()'s Content-Disposition and response-message handling operate on a realistic shape rather than a bare status code.
  • "Break this up into several test methods or use a data provider" — the single test_media_sideload_image() is now 12 focused test methods, three of which use data providers.
  • "Expected value should be passed as the first parameter" — all assertions are assertSame( $expected, $actual, $message ) with a failure message.

Also modernised along the way: set_up()/tear_down() instead of setUp()/tearDown(), assertSame() instead of assertEquals(), no assertRegExp()/assertInternalType() (removed in PHPUnit 9), array syntax per WPCS, and @covers ::media_sideload_image annotations.

### Testing instructions

npm run test:php -- --filter Tests_Admin_IncludesMedia

42 tests, 129 assertions pass. Commenting out add_post_meta( $id, '_source_url', $file ); in media_sideload_image() fails 3 of the new tests, confirming they exercise the behaviour the ticket is about. The full --group media suite (876 tests) also passes.

---
🤖 Generated with Claude Code

@mukesh27 commented on PR #190:


33 hours ago
#17

Closing in favour of #13397

Note: See TracTickets for help on using tickets.