Make WordPress Core

Opened 8 years ago

Closed 17 months ago

Last modified 17 months ago

#37930 closed enhancement (fixed)

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

Reported by: flixos90's profile flixos90 Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: Cc:

Description

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 7 years ago.
37930.2.diff (1.0 KB) - added by desrosj 7 years ago.
37930.patch (1016 bytes) - added by spacedmonkey 7 years ago.

Download all attachments as: .zip

Change History (32)

@flixos90
7 years ago

#1 @flixos90
7 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
7 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.


7 years ago

@desrosj
7 years ago

#4 @desrosj
7 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
7 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.


7 years ago

@spacedmonkey
7 years ago

#7 @spacedmonkey
7 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.


7 years ago

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

#41576 was marked as a duplicate.

#12 @SergeyBiryukov
21 months ago

#56045 was marked as a duplicate.

This ticket was mentioned in PR #2859 on WordPress/wordpress-develop by pbearne.


21 months ago
#13

  • Keywords has-unit-tests added

Reash and added test

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

#14 @spacedmonkey
20 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#15 @spacedmonkey
20 months ago

  • Milestone changed from Awaiting Review to 6.1

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


20 months ago

#17 @joyously
20 months ago

Does this play nice with the Customizer?

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


18 months ago

#19 @davidbaumwald
18 months ago

  • Keywords commit added

@spacedmonkey Is this still on your list to commit before 6.1 Beta 1? I am happy to commit this around the same time as #56045, since they are very similar.

#20 @spacedmonkey
18 months ago

@davidbaumwald if you want to commit, go for it.

#21 @davidbaumwald
18 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 54145:

Options, Meta APIs: Add a new pre-option filter.

Although a pre_option_{$option} filter already exists, this change adds a more general pre_option filter that will run on every get_option call. This brings the control flow into similar flow as update_option.

Props flixos90, NathanAtmoz, desrosj, spacedmonkey, pbearne.
Fixes #37930.

#23 @SergeyBiryukov
18 months ago

In 54147:

Tests: Rename the test for pre_option filter to match the filter name.

Move the method to a more appropriate place, next to the test for default_option_* filter.

Follow-up to [54145].

See #37930.

#24 @milana_cap
17 months ago

  • Keywords needs-dev-note add-to-field-guide added

This one can either go into grouped Dev note or just in Field guide.

#25 @spacedmonkey
17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#26 @spacedmonkey
17 months ago

  • Owner changed from spacedmonkey to davidbaumwald
  • Status changed from reopened to assigned

#27 @spacedmonkey
17 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#28 @milana_cap
17 months ago

  • Keywords add-to-field-guide removed
Note: See TracTickets for help on using tickets.