Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#34689 closed defect (bug) (wontfix)

update_option should not trigger the option_{option name} filter

Reported by: mark-k's profile mark-k Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

example code will illustrate the problem best

function my-option($v) {
  if (is_single() {
    return '1';
  }

  return $v;
}

add_filter('option_my_option','my_option');

now somewhere in a single.php of a theme

update_option('my_option','1');

will not write to the DB since the code will detect that the value wasn't changed.

I guess this is an extreme rare case in practice, but it seems like either get_option should get a 'raw' parameter and ignore filters, or update_option should get the raw value in another way.

Change History (3)

#1 @boonebgorges
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

This seems like a minefield to me. The get_option() check in update_option() is intended to check whether the provided $value is the same as the existing value of the option. If you have a filter like your my_option(), then, from WP's point of view, the value of 'my_option' *is* '1'. In your specific case, the filter is conditional - if ( is_single() ). But, in general, how can WP know whether the value returned by the 'option_' filter is the "real" value? It's likely that people are using filters like this to *prevent* database hits.

I'd suggest that conditional filters on 'option_' point toward architectural problems in what you're building. You really should be filtering something further up the stack. If WP is missing an appropriate filter for your purpose, please feel free to request it :)

#2 @mark-k
9 years ago

maybe it is a minefield, but basically you say that the option_{option name} filter should never be used.

The "existing value" checked against should be the value in the DB as the whole point of the check is to save a DB write. You can't say that the validation should be against get_option because the code is written that way as it is a pointless circular logic.

I have stepped over it as I was writing unit tests and produced some execution path that is unlikely to happen in real life, but "unlikely != will never".

I'd suggest that conditional filters on 'option_' point toward architectural problems in what you're building. You really should be filtering something further up the stack. If WP is missing an appropriate filter for your purpose, please feel free to request it :)

Sure, I agree that filtering at the appropriate functional level is preferred over filtering low level but

  1. There is no higher level to 'comment_registration'.
  2. the bug is still there for other people to stumble on it.

IMO getting the "get the raw value" functionality to a common function which will be used by both get_option and update_option should be very low risk thing. It is higher risk to keep this bug which at best case will waste people's time and in worst will produce un debugable WTF moments.

#3 @mark-k
9 years ago

An even more obvious solution is to not attempt to save the DB write. It is not like this check actually save in practice any detectable CPU time.

Note: See TracTickets for help on using tickets.