#32351 closed defect (bug) (fixed)
sanitize_option desperately needs unit tests
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Formatting | Keywords: | has-patch dev-feedback |
Focuses: | Cc: |
Attachments (4)
Change History (19)
#3
@
10 years ago
Added some basic tests (19 of them). Could still use a few more specific ones. I'll come back and add more.
#5
@
10 years ago
- Keywords needs-patch added; has-patch dev-feedback removed
This could be reduced down to one test with a data provider, for brevity.
jeremyfelt actually did this recently for update_site_option()
sanitization in [32634].
#7
in reply to:
↑ 6
@
10 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
Replying to obenland:
Patch updated to use a dataprovider. If this is an acceptable approach, we should probably add in some tests that show how the function handles bad input for the original set of tests.
We're you assigning me as the owner @obenland? If so, thanks!
This ticket was mentioned in Slack in #core by welcher. View the logs.
10 years ago
#10
@
10 years ago
- Milestone changed from Future Release to 4.3
- Owner changed from welcher to johnbillion
- Status changed from assigned to accepted
#11
@
10 years ago
- Keywords good-first-bug removed
32351.4.diff is an updated patch which:
- Uses strict comparison (
$this->assertSame()
) to avoid ambiguity with empty values - Adds tests for invalid values
- Adds tests for values that mutate when they're sanitized
One concern I have is that these tests don't actually test what they need to test in order to uncover problems with $wpdb->strip_invalid_text_for_column()
. We have unit test coverage for that method in Tests_DB_Charset
, but is it enough?
#14
@
10 years ago
- Resolution set to fixed
- Status changed from accepted to closed
Tests are in. As per my comment above, I'm still concerned that these tests don't actually cover funky behaviour of $wpdb->strip_invalid_text_for_column()
.
I'll open a follow-up ticket for improvements to Tests_DB_Charset
.
Deleting milestone since we don't have a patch for this yet, but it should really be handled as soon as possible.