Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#32351 closed defect (bug) (fixed)

sanitize_option desperately needs unit tests

Reported by: chriscct7's profile chriscct7 Owned by: johnbillion's profile 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 10 years ago.
32351.2.diff (4.5 KB) - added by welcher 10 years ago.
Fixed Redeclaration error and adding test for upload_path
32351.3.diff (2.5 KB) - added by welcher 10 years ago.
Updated existing tests to use a dataprovider
32351.4.diff (4.2 KB) - added by johnbillion 10 years ago.

Download all attachments as: .zip

Change History (19)

#1 @chriscct7
10 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
10 years ago

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

@MikeHansenMe
10 years ago

#3 @MikeHansenMe
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.

@welcher
10 years ago

Fixed Redeclaration error and adding test for upload_path

#4 @welcher
10 years ago

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

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

#6 follow-up: @obenland
10 years ago

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

@welcher
10 years ago

Updated existing tests to use a dataprovider

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

#8 @obenland
10 years ago

  • Owner changed from welcher 2 to welcher

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


10 years ago

#10 @johnbillion
10 years ago

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

@johnbillion
10 years ago

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

#12 @obenland
10 years ago

@johnbillion can you bring this over the finish line?

#13 @johnbillion
10 years ago

In 33119:

Introduce unit tests for sanitize_option().

Props MikeHansenMe, welcher, johnbillion
See #32351

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

Note: See TracTickets for help on using tickets.