Make WordPress Core

Opened 5 years ago

Last modified 4 years ago

#37930 new enhancement

Introduce a general `pre_option` filter in `get_option()`

Reported by: flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch
Focuses: Cc:


We currently have a pre_option_{$option} filter in get_option() to short-circuit the process of retrieving an option. I think it would make sense to also add a global filter pre_option, similar like we have in update_option().

Attachments (3)

37930.diff (971 bytes) - added by flixos90 5 years ago.
37930.2.diff (1.0 KB) - added by desrosj 4 years ago.
37930.patch (1016 bytes) - added by spacedmonkey 4 years ago.

Download all attachments as: .zip

Change History (14)

5 years ago

#1 @flixos90
5 years ago

  • Keywords has-patch added; needs-patch removed

37930.diff adds a filter pre_get_option. It works similar like the new filters proposed in #37928 and #37929 respectively.

#2 @NathanAtmoz
4 years ago

I think this makes sense. I just have a question about the order.

The get_option() function filters pre_update_option_{$option} before pre_update_option, so would it make more sense to keep the same order in get_option()?

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.

4 years ago

4 years ago

#4 @desrosj
4 years ago

Made an adjustment on the patch. The filter above it checks for false !== $value where the first patch checked for ! is_null(). Changed that for consistency.

Also, updated the PHPDocblock to reflect 4.8.0, and fix a typo.

#5 @desrosj
4 years ago

I looked at this one first of the three, of course (#37929 and #37928), and the other two check for null.

I did some digging because I feel like most of the "short circuit" filters I have used in core have a truthy check for false !== $value instead of null. It seems there is a pretty equal number throughout core for each. Here are some examples.

Truthy check

Locations using a null !== $value check:

I think that we should at least remain consistent in the options API.

Looking through options.php, I see the following:

Truthy checks already in core:

  • pre_site_transient_{$transient}
  • pre_option_{$option}
  • pre_get_option - addressed in this patch
  • pre_transient_{$transient},
  • pre_site_option_{$option}

There are currently only two null !== $value or ! is_null() checks in options.php, and both follow database queries that more than likely return null.

I vote for false !== checks, unless there are reasons why null is beter that I don't know of.

This ticket was mentioned in Slack in #core-multisite by desrosj. View the logs.

4 years ago

4 years ago

#7 @spacedmonkey
4 years ago

Updated this patch with another solution. Consider the following code in update_option

        $value = apply_filters( "pre_update_option_{$option}", $value, $old_value, $option );

         * Filters an option before its value is (maybe) serialized and updated.
         * @since 3.9.0
         * @param mixed  $value     The new, unserialized option value.
         * @param string $option    Name of the option.
         * @param mixed  $old_value The old option value.
        $value = apply_filters( 'pre_update_option', $value, $option, $old_value );

It passes the value through both filters. I have updated my patch so follow this patten. My patch passes the variable through the pre_get_option second and uses the existing return.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.

4 years ago

#10 @sc0ttkclark
4 years ago

I'd like to also see general filters implemented for default_option in get_option() as well, used in three potential places for return there.

#11 @SergeyBiryukov
4 years ago

#41576 was marked as a duplicate.

Note: See TracTickets for help on using tickets.