WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 4 months ago

Last modified 4 months ago

#26394 closed enhancement (fixed)

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

Reported by: codix Owned by: boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.7
Component: Options, Meta APIs Keywords: has-patch
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 19 months ago.
26394.diff (400 bytes) - added by nofearinc 19 months ago.
patch refresh
26394.2.diff (2.0 KB) - added by MikeHansenMe 4 months ago.
26394.3.diff (3.3 KB) - added by MikeHansenMe 4 months ago.
Added unit tests and docs.
26394.4.diff (3.7 KB) - added by MikeHansenMe 4 months ago.

Download all attachments as: .zip

Change History (22)

@codix19 months ago

comment:1 @SergeyBiryukov19 months ago

  • Version changed from trunk to 3.7

@nofearinc19 months ago

patch refresh

comment:2 @nofearinc19 months 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.

comment:3 @uglyrobot19 months 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.

comment:4 @jartes18 months ago

  • Cc joan@… added

comment:5 @nacin18 months ago

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

@MikeHansenMe4 months ago

comment:6 @MikeHansenMe4 months 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 4 months ago by DrewAPicture (previous) (diff)

comment:7 @DrewAPicture4 months 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.

@MikeHansenMe4 months ago

Added unit tests and docs.

comment:8 @boonebgorges4 months 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'.

comment:9 @MikeHansenMe4 months ago

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

@MikeHansenMe4 months ago

comment:10 @MikeHansenMe4 months 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

comment:11 @boonebgorges4 months 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.

comment:12 @boonebgorges4 months 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.

comment:13 @dd324 months 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.

comment:14 @DrewAPicture4 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:15 @boonebgorges4 months 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.

comment:16 @boonebgorges4 months 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.

comment:17 @DrewAPicture4 months ago

#18244 was marked as a duplicate.

Note: See TracTickets for help on using tickets.