WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#32351 closed defect (bug) (fixed)

sanitize_option desperately needs unit tests

Reported by: chriscct7 Owned by: johnbillion
Milestone: 4.3 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Sanitize_option, one of the more critical functions in WordPress, currently has no unit tests per discovery in #32308. We need to add these as soon as possible. Related #32350

Good first bug for someone who wants to start it. Probably requires more experience to finish.

Attachments (4)

32351.diff (4.1 KB) - added by MikeHansenMe 6 years ago.
32351.2.diff (4.5 KB) - added by welcher 6 years ago.
Fixed Redeclaration error and adding test for upload_path
32351.3.diff (2.5 KB) - added by welcher 6 years ago.
Updated existing tests to use a dataprovider
32351.4.diff (4.2 KB) - added by johnbillion 6 years ago.

Download all attachments as: .zip

Change History (19)

#1 @chriscct7
6 years ago

  • Milestone 4.3 deleted

Deleting milestone since we don't have a patch for this yet, but it should really be handled as soon as possible.

#2 @helen
6 years ago

  • Milestone set to Future Release
  • Type changed from task (blessed) to defect (bug)

@MikeHansenMe
6 years ago

#3 @MikeHansenMe
6 years ago

Added some basic tests (19 of them). Could still use a few more specific ones. I'll come back and add more.

@welcher
6 years ago

Fixed Redeclaration error and adding test for upload_path

#4 @welcher
6 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

#5 @johnbillion
6 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].

#6 follow-up: @obenland
6 years ago

  • Owner set to welcher 2
  • Status changed from new to assigned

@welcher
6 years ago

Updated existing tests to use a dataprovider

#7 in reply to: ↑ 6 @welcher
6 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!

#8 @obenland
6 years ago

  • Owner changed from welcher 2 to welcher

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


6 years ago

#10 @johnbillion
6 years ago

  • Milestone changed from Future Release to 4.3
  • Owner changed from welcher to johnbillion
  • Status changed from assigned to accepted

@johnbillion
6 years ago

#11 @johnbillion
6 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?

#12 @obenland
6 years ago

@johnbillion can you bring this over the finish line?

#13 @johnbillion
6 years ago

In 33119:

Introduce unit tests for sanitize_option().

Props MikeHansenMe, welcher, johnbillion
See #32351

#14 @johnbillion
6 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.

Note: See TracTickets for help on using tickets.