Make WordPress Core

Opened 4 years ago

Last modified 5 months ago

#52723 new defect (bug)

Admin options.php default value to NULL for option_value may lead to MySQL Integrity constraint violation error, potential other bugs

Reported by: pinoceniccola's profile pinoceniccola Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.7
Component: Options, Meta APIs Keywords: has-patch needs-unit-tests early
Focuses: administration Cc:

Description

It looks like wp-admin/options.php set a null value by default for any unchecked option:

https://core.trac.wordpress.org/browser/trunk/src/wp-admin/options.php#L306

Now, this leads to execute queries like this by update_option:

UPDATE wp_options SET option_value = NULL WHERE option_name = 'default_pingback_flag'

Which is wrong, given the schema explicitly set option_value to NOT NULL:

https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/schema.php#L144

This would trigger an integrity constraint violation error by MySQL when in (default) strict mode:

Error! SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'option_value' cannot be null

To get around this (and for other reasons too, I presume), WordPress currently tries to disable any MySQL strict mode in the $wpdb class, with the effect that MySQL silently "fix" the error itself:

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/wp-db.php#L567

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/wp-db.php#L826

But not every environment support this, so there are people out there who cannot save options and they are confused about the reason why, for example:

https://www.reddit.com/r/Wordpress/comments/l61rvs/cannot_disable_avatars/
https://wordpress.org/support/topic/discussion-comment-settings-saved-changes-are-not-taking-effect-at-all/
https://wordpress.org/support/topic/wordpress-database-error-column-option_value-cannot-be-null/

A simple solution would be to set a different default value (0 or even an empty string) in wp-admin/options.php and, better yet, cast any NULL value to the same different default value in both update_option and add_option.

Please note that, without a fix, this bug may also lead to other nasty side effects.

As a quick fix/test, I successful got around this with this simple filter:

<?php
add_action( 'init', 'p_options_fix' );

function p_options_fix() {

        add_filter( 'pre_update_option', 'p_options_fix_not_null', 10, 3 );

        function p_options_fix_not_null( $value, $option, $old_value ) {

                // The fix: cast NULL values to 0
                $value = ( is_null($value) ) ? 0 : $value;

                return $value;
        }
}


But I think this is something that should be really fixed in the core.

Thank you.

Change History (9)

This ticket was mentioned in PR #1074 on WordPress/wordpress-develop by pinoceniccola.


4 years ago
#1

  • Keywords has-patch added

Replaces null value as default admin option value to avoid bugs and side-effects. See ticket for details.

Trac ticket: https://core.trac.wordpress.org/ticket/52723

#2 @xmic
10 months ago

  • Component changed from General to Options, Meta APIs
  • Version set to trunk

A very warm welcome @pinoceniccola ! 👋

We've also encountered this issue on HyperDB since it didn't seem to save NULL option values (see the HyperDB 1.4 changelog).

Therefore, we have followed the solution to "cast NULL to zero" and it worked as intended.

<?php

/**
 * Casts NULL values to 0 for HyperDB.
 *
 * HyperDB currently doesn't support NULL values, so this function is used to cast any NULL values to 0.
 *
 * @param  mixed  $value     The value to be checked and possibly cast to 0.
 * @param  string $option    The name of the option being updated.
 * @param  mixed  $old_value The old value of the option.
 * @return mixed  The new, possibly cast, value.
 *
 * @see https://core.trac.wordpress.org/ticket/52723
 * @see HyperDB Changelog 1.4 here: https://github.com/Automattic/HyperDB
 * 
 * @since HyperDB 1.9
 */
function cast_null_option_to_zero( $value, $option, $old_value ) {

        // Bail early if HyperDB isn't installed.
        if ( ! class_exists( 'hyperdb' ) ) {
                return;
        }

        // Cast NULL values to 0.
        $value = ( is_null( $value ) ) ? 0 : $value;

        return $value;
}

add_filter( 'pre_update_option', 'cast_null_option_to_zero', 10, 3 );

Kind regards,
Michael

#3 @swissspidy
9 months ago

  • Keywords needs-unit-tests needs-testing added
  • Milestone changed from Awaiting Review to 6.6
  • Version changed from trunk to 2.7

Introduced in [9506].

Casting to 0 (zero) sounds odd to me, an empty string seems to make more sense.

This ticket was mentioned in PR #6704 on WordPress/wordpress-develop by @shailu25.


5 months ago
#4

Trac Ticket: 52723

#5 @shailu25
5 months ago

Refreshed the patch & added Empty String Instead of 0 in Default Option.

#6 in reply to: ↑ description @SergeyBiryukov
5 months ago

Replying to pinoceniccola:

A simple solution would be to set a different default value (0 or even an empty string) in wp-admin/options.php and, better yet, cast any NULL value to the same different default value in both update_option and add_option.

Hi there, welcome to WordPress Trac! Thanks for the ticket.

As previously noted in comment:6:ticket:57728, it appears that changing this default might have unintended consequences, as some options, e.g. blog_public, actually rely on null being passed to update_option() if the option was not sent to options.php, see [21849] and [21851] / #16416.

Related: [56132] / #57728.

#7 @xmic
5 months ago

Thanks @SergeyBiryukov

We may want to remove the reliance on the null check for the option/s, and instead use empty() to check if the value is either null or an empty string ''.

That way, it becomes clearer to check for an empty value without reliance on a specific type.

Any thoughts?

<?php

$a = empty(null);
echo var_export($a); // true

$a = empty('');
echo var_export($a); // true

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


5 months ago

#9 @oglekler
5 months ago

  • Keywords early added; needs-testing removed
  • Milestone changed from 6.6 to Future Release

This ticket was discussed during bug scrub.

It needs consensus on a way forward first and then likely needs to happen earlier in the cycle.

I am moving it to the Future Release until there will be a clear way forward.

Add props @hellofromTonya and @costdev

Note: See TracTickets for help on using tickets.