WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 6 months ago

Last modified 11 days 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 21 months ago.
26394.diff (400 bytes) - added by nofearinc 21 months ago.
patch refresh
26394.2.diff (2.0 KB) - added by MikeHansenMe 6 months ago.
26394.3.diff (3.3 KB) - added by MikeHansenMe 6 months ago.
Added unit tests and docs.
26394.4.diff (3.7 KB) - added by MikeHansenMe 6 months ago.

Download all attachments as: .zip

Change History (23)

@codix21 months ago

comment:1 @SergeyBiryukov21 months ago

  • Version changed from trunk to 3.7

@nofearinc21 months ago

patch refresh

comment:2 @nofearinc21 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 @uglyrobot21 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 @jartes20 months ago

  • Cc joan@… added

comment:5 @nacin20 months ago

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

@MikeHansenMe6 months ago

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

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

@MikeHansenMe6 months ago

Added unit tests and docs.

comment:8 @boonebgorges6 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 @MikeHansenMe6 months ago

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

@MikeHansenMe6 months ago

comment:10 @MikeHansenMe6 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 @boonebgorges6 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 @boonebgorges6 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 @dd326 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 @DrewAPicture6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:15 @boonebgorges6 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 @boonebgorges6 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 @DrewAPicture6 months ago

#18244 was marked as a duplicate.

comment:18 @ocean9011 days 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.

Note: See TracTickets for help on using tickets.