Skip to content

Conversation

@aditigodse10
Copy link

This PR adds a sanitize_callback to the tenup_isc_most_read_palette option to validate input before saving.

✅ Accepts only true, 'on', 1, or '1'
❌ Rejects all invalid values
🛡️ Shows an admin error using add_settings_error() when input is incorrect

This ensures secure, expected handling of the checkbox setting and follows WordPress sanitization best practices.

@jeffpaul jeffpaul added this to the Future Release milestone Aug 11, 2025
@jeffpaul jeffpaul requested review from Sidsector9 and removed request for dkotter and jeffpaul August 11, 2025 18:33
@jeffpaul jeffpaul moved this to Code Review in Open Source Practice Aug 11, 2025
'default' => false,
'sanitize_callback' => function( $input ) {
// Accept only true, 'on', or 1
if ( $input === 'on' || $input === true || $input === 1 || $input === '1' ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for raising this fix @aditigodse10

I found an issue, that when you try to save the settings with the checkbox unchecked, the Invalid value for Most Used Palette setting. error is shown on top of the page. This is because $input is null when it is unchecked. Can you fix that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure, will do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aditigodse10 checking in to see if you'll be able to updated based on the code review feedback above?

@github-project-automation github-project-automation bot moved this from Code Review to In Progress in Open Source Practice Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants