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
@
6 years ago
- Summary Test coverage for → Test coverage for media_sideload_image() _source_url meta
This ticket was mentioned in Slack in #core by killua99. View the logs.
6 years ago
#4
@
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 PR #190 on WordPress/wordpress-develop by killua99.
6 years ago
#5
Trac ticket: https://core.trac.wordpress.org/ticket/49631
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
@
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
@
6 years ago
- Milestone 5.5 → 5.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
@
6 years ago
- Milestone 5.6 → Future 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. :)
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_urlpost 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. $descbeing used as escapedalttext 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\bin the regex) each return animage_sideload_failed/ "Invalid image URL." error *before* any HTTP request is made. An empty$fileis covered separately. - The
image_sideload_extensionsfilter — adding an extension (bmp) lets a matching URL through and still records_source_url, removingjpg/jpeg/jperejects one, and the filter receives the URL being sideloaded as its second argument. - Download failures — a transport
WP_Errorand 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 callsremove_added_uploads(), which *is* still required (WP_UnitTestCasesnapshots the uploads directory inset_up()but does not clear it), and matchesTests_Post_Attachments,Tests_Image_SiteIconand others. Theremove_filter()call is dropped — hooks are restored between tests. - "This would need to return more than just a response code" — the
pre_http_requestmock now returns a complete response array (headers,body,response,cookies,filename) sodownload_url()'sContent-Dispositionand 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
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
Assigning to @killua99 for a first pass.
Note that you can write a test using
WP_TESTS_DOMAINto sideload from the site set up during testing, rather than an external source.