Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 7 years ago

#26394 closed enhancement (fixed)

In update_option if option is missing add the option with autoload no

Reported by: codix's profile codix Owned by: boonebgorges's profile boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.7
Component: Options, Meta APIs Keywords: has-patch 2nd-opinion
Focuses: performance Cc:

Description

Right now if an option is not added with add_option and subsequently updated it will be added as autoload yes. This would work fine in most cases. But it becomes a problem if the total size of autoload options exceed 1024*1024-42 bytes with memecache with default configuration and object cache with memcache backend. Also storing 1024*1024 bytes of data per blog in a very large multi site environment is not best use of resources.

I propose update_option to add options with autoload no as many developers just use update_option to even add options.

Attachments (5)

patch.diff (726 bytes) - added by codix 10 years ago.
26394.diff (400 bytes) - added by nofearinc 10 years ago.
patch refresh
26394.2.diff (2.0 KB) - added by MikeHansenMe 9 years ago.
26394.3.diff (3.3 KB) - added by MikeHansenMe 9 years ago.
Added unit tests and docs.
26394.4.diff (3.7 KB) - added by MikeHansenMe 9 years ago.

Download all attachments as: .zip

Change History (25)

@codix
10 years ago

#1 @SergeyBiryukov
10 years ago

  • Version changed from trunk to 3.7

@nofearinc
10 years ago

patch refresh

#2 @nofearinc
10 years ago

I've uploaded a SVN diff of the patch for compatibility. Did you use git diff --no-prefix to get the patch as it should be the right way to do it?

Additionally, I agree with your concerns, but given the fact that add_option sets autoload as yes by default, I'm not sure if that the patch is not against the defaults expected in add_option.

#3 @uglyrobot
10 years ago

I agree that 'yes' should be the default, but it would be AWESOME to be able to add an autoload argument to update_option(). As said almost no one uses add_option() first.

#4 @jartes
10 years ago

  • Cc joan@… added

#5 @nacin
10 years ago

  • Component changed from Performance to Options and Meta
  • Focuses performance added

@MikeHansenMe
9 years ago

#6 @MikeHansenMe
9 years ago

  • Keywords has-patch added

26394.2.diff adds an additional argument that has a default of 'yes'. This will allow existing code to continue working as it has but allows specific 'no' cases to add the additional arg.

Last edited 9 years ago by DrewAPicture (previous) (diff)

#7 @DrewAPicture
9 years ago

  • Keywords needs-docs needs-unit-tests added

The new parameter is optional so that should be reflected in the parameter description. We'll also need to add a changelog entry to the DocBlock, e.g.

@since 4.2.0 The `$autoload` parameter was added.

Also, we probably need unit tests for this.

@MikeHansenMe
9 years ago

Added unit tests and docs.

#8 @boonebgorges
9 years ago

  • Keywords needs-patch added; has-patch needs-docs needs-unit-tests removed

MikeHansenMe - Thanks for the tests, but they'll need to be a bit more specific. The tests should test that autoload is set correctly for the new options - you should be able to check this with wp_load_alloptions() and some sort of direct cache check, or perhaps by checking $wpdb->num_queries before and after a get_option() call. In particular, it'd be great to see a test that demonstrates how the default value of $autoload is 'yes'.

#9 @MikeHansenMe
9 years ago

@boonebgorges I will get some better unit tests written for this.

@MikeHansenMe
9 years ago

#10 @MikeHansenMe
9 years ago

  • Keywords has-patch added; needs-patch removed

26394.4.diff changes the tests to check query count. autoload = 'yes' increments by 1 because of query:

SELECT option_name, option_value FROM wptests_options WHERE autoload = 'yes'

the additional query from the autoload = 'no' test is

SELECT option_value FROM wptests_options WHERE option_name = 'test_update_option_default' LIMIT 1

#11 @boonebgorges
9 years ago

  • Milestone changed from Awaiting Review to 4.2

Thank you, MikeHansenMe - I'm going to do a bit of cleanup (there is some ugliness in MS tests related to #31130) and go with it.

#12 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 31628:

Introduce $autoload parameter to update_option().

When creating an option via add_option(), the $autoload param allows you to
tell WP whether the option should be loaded as part of the 'alloptions' cache
during every pageload. update_option(), when used with a non-existent option
calls add_option() internally. The new $autoload param in update_option()
is passed along to add_option() in cases where the option does not yet exist.

The associated unit tests are skipped on multisite due to an issue that causes
WP_INSTALLING to force cache misses. See #31130.

Props codix, nofearinc, MikeHansenMe.
Fixes #26394.

#13 @dd32
9 years ago

This change feels strange, I think most developers will expect that passing this parameter will update the autoload status in the database for an existing option.

I think we really need to handle passing it for an existing option. We could reduce the weight of that change by requiring the value changes as part of the update.

#14 @DrewAPicture
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#15 @boonebgorges
9 years ago

dd32 - It never dawned on me that one would want to use this to change autoload for existing options, but on reflection I totally agree.

We could reduce the weight of that change by requiring the value changes as part of the update.

Yeah. There would be a lot of questions about return values and hooks if it were possible to update autoload without also updating value. I'll try to make the requirement clear in the docs.

#16 @boonebgorges
9 years ago

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

In 31640:

Allow $autoload setting to be changed for existing options using update_option().

[31628] made it possible to pass an $autoload param to update_option() that
applies when the option does not yet exist in the database. The current
changeset introduces parity for existing options: the $autoload setting
for existing options can be changed via the $autoload parameter. For internal
simplicity, $autoload is ignored for existing options when $value is not
also changed.

This changeset also moves update_option() tests into their own class.

Props dd32.
Fixes #26394.

#17 @DrewAPicture
9 years ago

#18244 was marked as a duplicate.

#18 @ocean90
8 years ago

In 33703:

Tests: Fix a typo in function names for $autoload tests.

Autoload is "no" if the $autoload param for update_option() is 'no' or false.

see #26394.

#19 @westonruter
8 years ago

In 35305:

Customizer: Allow new option settings to not be saved as autoloaded by passing an autoload arg value of false.

The autoload argument value is passed along to update_option() which has accepted an $autoload parameter since [31628].

Props westonruter, dlh.
See #26394.
Fixes #33499.

#20 @theode
7 years ago

  • Keywords 2nd-opinion added

I want to know why we not set update_option()'s $autload parameter to "false" by default? because it broke the #WordPress after 260'000 entries in wp_options with $autoload = true, simply because developer forgot to set it to false.

wp-include/option.php tried to load them all

Last edited 7 years ago by theode (previous) (diff)
Note: See TracTickets for help on using tickets.